Pr/13 UI usability

docs/PR13-REVIEW.md

@@ -0,0 +1,108 @@
+# PR #13 Review - "Improve UI usability for new users"
+
+Reviewer: Codex  
+Branch reviewed: `pr/13-ui-usability` (`99a9295`)  
+Base compared: `origin/master` (`21cd0c8`)  
+Changed files reviewed: `index.html`, `css/layout.css`, `css/components.css`, `js/app.js`, `js/components/dashboard.js`, `js/components/rig-list.js`
+
+## Executive Summary
+
+**Recommendation: REQUEST CHANGES (do not merge as-is).**
+
+This PR introduces a **blocking JavaScript syntax error** in `dashboard.js` that breaks module parsing and prevents the dashboard code from loading. It also introduces new "Add Your First Rig" CTAs that are not wired to modal opening in dynamic content, plus UX regressions in navigation state visibility and unread-mail discoverability.
+
+## Bugs Found
+
+### Critical
+
+1. **Syntax error: missing closing brace in `renderGettingStarted()` breaks file parsing**
+   - File: `js/components/dashboard.js:246-292` (missing `}` before `calculateMetrics`)
+   - Symptom: `node --check js/components/dashboard.js` => `SyntaxError: Unexpected end of input` (at EOF).
+   - Impact: Frontend module import fails for this file; dashboard functionality is broken and app boot may fail depending on module load path.
+   - Regression check: `origin/master` version parses cleanly.
+
+2. **New CTA buttons in dynamic content are not wired to modals**
+   - Files:
+     - `js/components/dashboard.js:258` (`data-modal-open="new-rig"`)
+     - `js/components/rig-list.js:31` (`data-modal-open="new-rig"`)
+     - Wiring limitation: `js/components/modals.js:103-107` binds click listeners only once at init to currently existing `[data-modal-open]` nodes.
+   - Impact: Newly added dynamic "Add Your First Rig" buttons are non-functional (no modal opens).
+   - Root cause: No delegated modal-open click handling for content rendered after `initModals()`.
+
+### Minor / UX Regressions
+
+3. **No persistent active-state indicator for dropdown views**
+   - Files:
+     - `index.html:82-117` (views moved under dropdown)
+     - `js/app.js:256-262` (active class toggled on `.nav-tab` and `.nav-dropdown-item`)
+     - `css/layout.css:369-372` (`.nav-dropdown-item.active` styling only visible when menu is open)
+   - Impact: When user is on `mail`, `health`, `issues`, etc., there is no visible active tab in the collapsed header unless the dropdown is opened. Orientation suffers.
+
+4. **Unread mail badge visibility regression**
+   - File: `index.html:107-111`
+   - Impact: Badge is now hidden inside the dropdown menu by default, reducing visibility of unread messages compared to always-visible top-nav badge.
+
+## Security Risks
+
+- **No new direct XSS vector identified in this PR diff** (new inserted content is static strings, not untrusted user data).
+- **Low-risk maintainability/security concern:** new inline handler (`js/components/dashboard.js:262`, `onclick="window.gastown?.startOnboarding?.()"`) increases reliance on inline JS and global namespace coupling. This is weaker for CSP hardening and can drift into unsafe patterns if later interpolated.
+
+## UX Assessment
+
+Positive intent:
+- Simplified top-level nav and "More" grouping can reduce top-bar clutter.
+- Empty states and getting-started guidance are directionally good for new users.
+
+Problems that currently block UX value:
+- Critical parser break means usability improvements do not reliably ship.
+- Primary new onboarding CTA path (Add Rig) is non-functional in dynamic areas.
+- Active view feedback is weaker for dropdown views.
+- Mail urgency signal is less discoverable.
+
+## Performance Assessment
+
+- No major performance regressions observed in changed code paths.
+- Minor note: additional `querySelectorAll` calls in navigation click handlers are small and acceptable for current DOM size.
+
+## Design Consistency
+
+- Mostly uses existing token system (`--space-*`, `--bg-*`, `--radius-*`, etc.).
+- Some newly added gradients use hard-coded RGBA values (`css/components.css:1206`, `css/components.css:5926`) rather than theme tokens; acceptable but less consistent with token-driven theming.
+
+## Line-by-Line Issues
+
+1. `js/components/dashboard.js:246-292`  
+   Missing closing brace for `renderGettingStarted()`. Parsing fails at EOF.
+
+2. `js/components/dashboard.js:258`  
+   `data-modal-open="new-rig"` button is dynamically rendered but not auto-wired by modal system.
+
+3. `js/components/rig-list.js:31`  
+   Same modal wiring gap for dynamic empty-state CTA button.
+
+4. `js/components/modals.js:103-107`  
+   Modal opening listeners are bound only to elements present during init; dynamic buttons require delegated handling.
+
+5. `js/app.js:256-262` + `css/layout.css:369-372` + `index.html:82-117`  
+   Active state is only visible on dropdown items inside closed menu; no persistent "current view" signal on the More toggle.
+
+6. `index.html:107-111`  
+   Mail badge moved into dropdown item, making unread counts invisible unless menu is opened.
+
+## Recommended Fixes Before Merge
+
+1. Add the missing `}` in `renderGettingStarted()` and re-run syntax check.
+2. Implement delegated modal-open handling (or explicit handlers) so dynamically rendered `[data-modal-open]` buttons work.
+3. Add visible active-state feedback on the `More` toggle when any dropdown child view is active.
+4. Restore persistent unread-mail indicator outside the dropdown (or mirror it on the `More` toggle).
+5. Replace inline `onclick` onboarding trigger with JS event binding in component setup for consistency and safer CSP posture.
+6. Re-run full tests once dependencies are installed.
+
+## Verification Commands Run
+
+- `git diff --name-status origin/master...HEAD`
+- `node --check js/app.js`
+- `node --check js/components/rig-list.js`
+- `node --check js/components/dashboard.js` (fails)
+- `git show origin/master:js/components/dashboard.js > /tmp/dashboard.master.js && node --check /tmp/dashboard.master.js` (passes)
+- `npm test` (could not run: `vitest: not found` in environment)

index.html

@@ -83,6 +83,7 @@
             <button class="nav-tab nav-dropdown-toggle" data-tooltip="More options">
               <span class="material-icons">more_horiz</span>
               <span class="nav-label">More</span>
+              <span class="badge hidden" id="more-badge">0</span>
               <span class="material-icons nav-dropdown-arrow">expand_more</span>
             </button>
             <div class="nav-dropdown-menu">

js/app.js

@@ -50,6 +50,7 @@ const elements = {
   townName: document.getElementById('town-name'),
   connectionStatus: document.getElementById('connection-status'),
   mailBadge: document.getElementById('mail-badge'),
+  moreBadge: document.getElementById('more-badge'),
   hookStatus: document.getElementById('hook-status'),
   statusMessage: document.getElementById('status-message'),
   agentTree: document.getElementById('agent-tree'),
@@ -257,8 +258,15 @@ function switchView(viewId) {
     tab.classList.toggle('active', tab.dataset.view === viewId);
   });
 
+  let hasActiveDropdownView = false;
   document.querySelectorAll('.nav-dropdown-item').forEach(item => {
-    item.classList.toggle('active', item.dataset.view === viewId);
+    const isActive = item.dataset.view === viewId;
+    item.classList.toggle('active', isActive);
+    if (isActive) hasActiveDropdownView = true;
+  });
+
+  document.querySelectorAll('.nav-dropdown-toggle').forEach(toggle => {
+    toggle.classList.toggle('active', hasActiveDropdownView);
   });
 
   views.forEach(view => {
@@ -762,8 +770,14 @@ function subscribeToState() {
 
     // Update badge
     const unread = mail.filter(m => !m.read).length;
-    elements.mailBadge.textContent = unread;
-    elements.mailBadge.classList.toggle('hidden', unread === 0);
+    if (elements.mailBadge) {
+      elements.mailBadge.textContent = unread;
+      elements.mailBadge.classList.toggle('hidden', unread === 0);
+    }
+    if (elements.moreBadge) {
+      elements.moreBadge.textContent = unread;
+      elements.moreBadge.classList.toggle('hidden', unread === 0);
+    }
   });
 }
 

js/components/dashboard.js

@@ -288,6 +288,7 @@ function renderGettingStarted() {
       </div>
     </div>
   `;
+}
 
 /**
  * Calculate dashboard metrics

js/components/modals.js

@@ -99,12 +99,13 @@ export function initModals() {
     btn.addEventListener('click', closeAllModals);
   });
 
-  // Open buttons
-  document.querySelectorAll('[data-modal-open]').forEach(btn => {
-    btn.addEventListener('click', () => {
-      const modalId = btn.dataset.modalOpen;
-      openModal(modalId);
-    });
+  // Open buttons (delegated so dynamically rendered content works too)
+  document.addEventListener('click', (e) => {
+    const btn = e.target.closest('[data-modal-open]');
+    if (!btn) return;
+    const modalId = btn.dataset.modalOpen;
+    if (!modalId) return;
+    openModal(modalId);
   });
 
   // Form submissions