fix: address Copilot review on #76 — first pass

Four findings, all valid:

1. `filterGearsByAgentCapabilities` was using `agent.NewClient` whose
   default timeout is 30s. That's on the critical path of every Gears
   settings page render — a dead agent would freeze the UI for 30s
   before the fail-open path took over. Now using
   `NewClientWithTimeout` with a 3s cap, more than enough for a
   healthy LAN agent.

2. `encodePagesJSON` (palette) and the `Settings()` template were
   independently spelling out which pages each user can reach. New
   `models.SettingsPagesFor(user, perms)` is now the single source of
   truth — both consumers iterate the same slice. Settings cards now
   come from a `settingsCard` template + name-keyed `settingsCardIcon`
   helper, so adding a new settings page is a one-line entry in
   `SettingsPagesFor` plus a switch case for the icon (forgetting the
   icon falls back to a generic glyph rather than crashing).

3. `services-config.js` silently no-op'd when `#services-config-root`
   had no `data-action` attribute. A future templ refactor that drops
   that attribute would have made every toggle look like it saved
   while doing nothing. Now logs a one-shot console.error + toast.

4. `GearUpdatePost`'s `r.ParseForm` and `GetGear` early-exit branches
   were always responding with a 303 redirect to HTML, even when the
   AJAX caller asked for JSON. The fetch client would follow the
   redirect, get HTML, and `r.json().catch(() => ({success: r.ok}))`
   would treat it as success. `wantsJSON` is now computed up front
   and a tiny `jsonError` closure routes every error path through the
   right format.

Author: sarg3nt

gearbox/internal/framework/handler/gears.go

@@ -7,6 +7,7 @@ import (
 	"net/url"
 	"sort"
 	"strings"
+	"time"
 
 	"github.com/go-chi/chi/v5"
 	"github.com/sarg3nt/gearbox/internal/framework/agent"
@@ -93,6 +94,12 @@ var dashboardGearToAgentGear = map[string]string{
 	database.GearOSUpdates:    "updates",
 }
 
+// capabilitiesFetchTimeout caps the synchronous Gears-page → agent call
+// so a dead agent doesn't stall page rendering for the full 30s default.
+// 3s is enough for a healthy LAN agent, short enough that operators don't
+// notice when an agent is gone.
+const capabilitiesFetchTimeout = 3 * time.Second
+
 // filterGearsByAgentCapabilities removes gears the agent has reported as
 // not-installed / inaccessible / disabled. Fail-open on any error: when the
 // agent is unreachable or running a version without the capabilities
@@ -104,7 +111,10 @@ func (h *Handler) filterGearsByAgentCapabilities(boxID string, gears []database.
 		return gears
 	}
 
-	client := agent.NewClient(serverConfig.AgentURL, serverConfig.APIKey)
+	// Short timeout — this call sits on the critical path of every Gears
+	// page render, so a 30s default would freeze the UI when the box is
+	// down. Fail-open if it times out.
+	client := agent.NewClientWithTimeout(serverConfig.AgentURL, serverConfig.APIKey, capabilitiesFetchTimeout)
 	caps, err := client.GetCapabilities()
 	if err != nil {
 		h.logger.Debug("capabilities fetch failed; showing all gears", "box_id", boxID, "error", err)
@@ -355,20 +365,38 @@ func (h *Handler) GearUpdatePost(w http.ResponseWriter, r *http.Request) {
 	boxID := h.resolveBoxIDFromRequest(r)
 	redirectBase := "/settings/gears/" + gearName + "?server=" + url.QueryEscape(boxID)
 
+	// Compute wantsJSON up front so every error path below (parse failures,
+	// gear lookup failures, validation errors, save failures) can respond
+	// in the format the caller actually expects. AJAX auto-savers (services,
+	// logs) request JSON; classic form POSTs accept HTML and get redirects.
+	wantsJSON := strings.Contains(r.Header.Get("Accept"), "application/json")
+	jsonError := func(status int, msg string) {
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(status)
+		_ = json.NewEncoder(w).Encode(map[string]any{"success": false, "error": msg})
+	}
+
 	if err := r.ParseForm(); err != nil {
+		if wantsJSON {
+			jsonError(http.StatusBadRequest, "Invalid form data")
+			return
+		}
 		http.Redirect(w, r, redirectBase+"&error="+url.QueryEscape("Invalid form data"), http.StatusSeeOther)
 		return
 	}
 
 	// Get current gear
 	gearItem, err := h.db.GetGear(boxID, gearName)
 	if err != nil || gearItem == nil {
+		if wantsJSON {
+			jsonError(http.StatusNotFound, "Integration not found")
+			return
+		}
 		http.Redirect(w, r, "/settings/gears?server="+url.QueryEscape(boxID)+"&error="+url.QueryEscape("Integration not found"), http.StatusSeeOther)
 		return
 	}
 
 	// Handle config update based on gear type
-	wantsJSON := strings.Contains(r.Header.Get("Accept"), "application/json")
 	var newConfig json.RawMessage
 	switch gearName {
 	case database.GearServices:
@@ -381,9 +409,7 @@ func (h *Handler) GearUpdatePost(w http.ResponseWriter, r *http.Request) {
 		err = h.saveLogSourcesConfig(boxID, r)
 		if err != nil {
 			if wantsJSON {
-				w.Header().Set("Content-Type", "application/json")
-				w.WriteHeader(http.StatusInternalServerError)
-				_ = json.NewEncoder(w).Encode(map[string]any{"success": false, "error": err.Error()})
+				jsonError(http.StatusInternalServerError, err.Error())
 				return
 			}
 			http.Redirect(w, r, redirectBase+"&error="+url.QueryEscape(err.Error()), http.StatusSeeOther)
@@ -414,9 +440,7 @@ func (h *Handler) GearUpdatePost(w http.ResponseWriter, r *http.Request) {
 
 	if err != nil {
 		if wantsJSON {
-			w.Header().Set("Content-Type", "application/json")
-			w.WriteHeader(http.StatusBadRequest)
-			_ = json.NewEncoder(w).Encode(map[string]any{"success": false, "error": err.Error()})
+			jsonError(http.StatusBadRequest, err.Error())
 			return
 		}
 		http.Redirect(w, r, redirectBase+"&error="+url.QueryEscape(err.Error()), http.StatusSeeOther)
@@ -427,9 +451,7 @@ func (h *Handler) GearUpdatePost(w http.ResponseWriter, r *http.Request) {
 	if err := h.db.SetGearConfig(boxID, gearName, newConfig, &user.ID); err != nil {
 		h.logger.Error("failed to update gear", "gear", gearName, "error", err)
 		if wantsJSON {
-			w.Header().Set("Content-Type", "application/json")
-			w.WriteHeader(http.StatusInternalServerError)
-			_ = json.NewEncoder(w).Encode(map[string]any{"success": false, "error": "Failed to save configuration"})
+			jsonError(http.StatusInternalServerError, "Failed to save configuration")
 			return
 		}
 		http.Redirect(w, r, redirectBase+"&error="+url.QueryEscape("Failed to save configuration"), http.StatusSeeOther)

gearbox/internal/framework/models/settings_pages.go

@@ -0,0 +1,90 @@
+package models
+
+// SettingsPage is a single entry on the /settings root grid. The Settings
+// template renders one card per entry; the Cmd+K palette serializes the
+// same slice into its `cmdk-pages` JSON island. Both consumers go through
+// SettingsPagesFor so permission gating lives in exactly one place — if
+// it drifted, the palette would either dangle 403 destinations or hide
+// pages users can in fact reach (PR #76 Copilot review).
+type SettingsPage struct {
+	// Name is the stable kebab-case slug used as the palette item id and
+	// as the icon-key switch on the Settings card renderer. Must be unique.
+	Name string
+	// Label is the user-facing title shown on the card and in the palette.
+	Label string
+	// Description is the one-line card subtitle. Empty for palette-only
+	// entries (none today, kept as an explicit option).
+	Description string
+	// Path is the destination URL.
+	Path string
+}
+
+// SettingsPagesFor returns the ordered list of settings pages the given
+// user can reach. The Settings template iterates this for card layout;
+// encodePagesJSON iterates the same slice to populate the Cmd+K palette.
+// Adding a new settings page = appending an entry here once.
+//
+// perms may be nil for users whose permission record hasn't loaded — we
+// treat that as "no extra permissions beyond role" so admins still see
+// everything they should.
+func SettingsPagesFor(user *User, perms *UserPermissions) []SettingsPage {
+	isAdmin := user != nil && user.IsAdmin()
+	hasPerm := func(c Component, p Permission) bool {
+		return perms != nil && perms.HasPermission(c, p)
+	}
+
+	pages := make([]SettingsPage, 0, 10)
+	if isAdmin || hasPerm(ComponentSettings, PermissionManageBoxes) {
+		pages = append(pages, SettingsPage{
+			Name:        "boxes",
+			Label:       "Boxes",
+			Description: "Manage monitored boxes",
+			Path:        "/settings/boxes",
+		})
+	}
+	if isAdmin || hasPerm(ComponentGears, PermissionManage) {
+		pages = append(pages, SettingsPage{
+			Name:        "gears",
+			Label:       "Gears",
+			Description: "Enable/disable features and configure gears",
+			Path:        "/settings/gears",
+		})
+	}
+	pages = append(pages, SettingsPage{
+		Name:        "profile",
+		Label:       "Profile",
+		Description: "Update your personal information and passkeys",
+		Path:        "/settings/profile",
+	})
+	if isAdmin || hasPerm(ComponentUsers, PermissionApproveUsers) {
+		pages = append(pages, SettingsPage{
+			Name:        "users",
+			Label:       "User Management",
+			Description: "Manage users and account requests",
+			Path:        "/settings/users",
+		})
+	}
+	if isAdmin {
+		pages = append(pages, SettingsPage{
+			Name:        "smtp",
+			Label:       "Email Settings",
+			Description: "Configure SMTP server for email notifications",
+			Path:        "/settings/smtp",
+		})
+	}
+	pages = append(pages, SettingsPage{
+		Name:        "backups",
+		Label:       "Database Backups",
+		Description: "Create and restore database backups",
+		Path:        "/settings/backup",
+	})
+	if isAdmin {
+		pages = append(pages, SettingsPage{
+			Name:        "permissions",
+			Label:       "Permission Management",
+			Description: "Manage granular permissions for users",
+			Path:        "/settings/permissions",
+		})
+	}
+	return pages
+}

gearbox/internal/framework/templates/layouts/base.templ

@@ -117,10 +117,11 @@ func encodeGearsJSON(ctx context.Context) string {
 }
 
 // encodePagesJSON serializes the user's reachable settings/profile pages
-// for the command palette. Each entry has the same shape as a gear: name,
-// label, path. Only pages the current user can actually visit are emitted
-// — admin-only items are filtered out for non-admins so the palette
-// doesn't dangle 403 destinations. Issue #71 item 4.
+// for the command palette. Source of truth is models.SettingsPagesFor —
+// the Settings template renders cards from the same slice, so the palette
+// can never dangle a 403 destination or omit a page the user can reach.
+// Augmented with palette-only entries (Settings root, Profile sub-pages,
+// Add-a-box) that don't need their own Settings card.
 func encodePagesJSON(ctx context.Context) string {
 	type lite struct {
 		Name  string `json:"name"`
@@ -129,35 +130,19 @@ func encodePagesJSON(ctx context.Context) string {
 	}
 	user, _ := auth.GetUserFromContext(ctx)
 	perms, _ := auth.GetUserPermissionsFromContext(ctx)
-	isAdmin := user != nil && user.IsAdmin()
-	hasPerm := func(c models.Component, p models.Permission) bool {
-		return perms != nil && perms.HasPermission(c, p)
-	}
-
-	out := make([]lite, 0, 12)
-	add := func(name, label, path string) {
-		out = append(out, lite{Name: name, Label: label, Path: path})
-	}
-
-	// Always available
-	add("settings", "Settings", "/settings")
-	add("profile", "Profile", "/settings/profile")
-	add("change-password", "Change password", "/settings/change-password")
-	add("backups", "Database backups", "/settings/backup")
 
-	if isAdmin || hasPerm(models.ComponentSettings, models.PermissionManageBoxes) {
-		add("boxes", "Boxes", "/settings/boxes")
-		add("boxes-new", "Add a box", "/settings/boxes/new")
-	}
-	if isAdmin || hasPerm(models.ComponentGears, models.PermissionManage) {
-		add("gears", "Gears", "/settings/gears")
-	}
-	if isAdmin || hasPerm(models.ComponentUsers, models.PermissionApproveUsers) {
-		add("users", "User management", "/settings/users")
-	}
-	if isAdmin {
-		add("smtp", "Email (SMTP) settings", "/settings/smtp")
-		add("permissions", "Permission management", "/settings/permissions")
+	pages := models.SettingsPagesFor(user, perms)
+	out := make([]lite, 0, len(pages)+4)
+	// Always reachable, but not their own Settings card.
+	out = append(out, lite{Name: "settings", Label: "Settings", Path: "/settings"})
+	out = append(out, lite{Name: "change-password", Label: "Change password", Path: "/settings/change-password"})
+	for _, p := range pages {
+		out = append(out, lite{Name: p.Name, Label: p.Label, Path: p.Path})
+		// Surface "Add a box" alongside the Boxes card for one-keystroke
+		// access from anywhere — only when the user can see Boxes itself.
+		if p.Name == "boxes" {
+			out = append(out, lite{Name: "boxes-new", Label: "Add a box", Path: "/settings/boxes/new"})
+		}
 	}
 
 	b, err := json.Marshal(out)