fix(#103): address Copilot review findings on PR #104 (round 2)

Addresses the unresolved threads on PR #104 raised across the recent
review passes. Each change is one-to-one with a finding:

- metrics.templ: in-templ fullscreen Esc handler now preventDefaults
  so it doesn't fall through to the global Esc → history.back().
- metrics-layout.js patchLayout(): check `res.ok` and console.warn on
  non-2xx so 401/403/413/500 saves no longer fail silently.
- metrics-layout.js applyCapabilityHiding(): switch the capability
  marker from `.hidden` to a dedicated `data-cap-hidden` attribute so
  fullscreen-driven `.hidden`s on neighbouring cards can't get
  mis-interpreted as capability hides. applyCapabilities() in the
  templ sets the new attribute; per-source templ cards carry the
  initial `data-cap-hidden="true"` so the marker is correct before
  the first applyCapabilities() pass runs.
- metrics-layout.js: snapshot the (x,y,w,h) of each tile we remove
  from the engine into `capHiddenSnapshots` and merge those entries
  into `captureLayout()`, so a tile's last-known position survives
  a capability flip → re-flip cycle.
- metrics-layout.js loadSavedLayout(): pass `addRemove:false` to
  `gs.load()` — without it GridStack yanks any DOM tile whose gs-id
  isn't in the loaded blob, which would permanently strip the
  capability-hidden DOM nodes we still need to re-attach later.
- metrics-layout.js: fix updateContainerHeight() comment to describe
  what the code actually does (no margin term).
- metrics.templ: rewrite the "waits on DOMContentLoaded" script-tag
  comment to match the actual `defer` execution semantics.
- api_metrics_layout.go: normalise omitted `w`/`h` (GridStack drops
  them when they equal the default of 1) before the positive check,
  so resizing a tile to 1×N or N×1 no longer fails validation. Plus
  add column-count validation (x < 12, x+w ≤ 12) so a misbehaving
  client can't persist tiles that will silently clamp on next
  render. Tests cover both new behaviours plus a regression for the
  omitted-w/h case.

Author: sarg3nt

gearbox/internal/framework/handler/api_metrics_layout.go

@@ -55,6 +55,12 @@ const maxTilesPerLayout = 64
 const (
 	maxCoord = 1000
 	maxDim   = 100
+	// gridColumns mirrors the GridStack init in static/js/metrics-layout.js
+	// (column: 12). Persisted x+w must fit inside this column count or
+	// the saved tile would silently collide / clamp on next render,
+	// which surfaces as "my layout didn't stick" confusion. Keep this
+	// value in sync with the JS side if the column count ever changes.
+	gridColumns = 12
 )
 
 // layoutTile is the shape we require each entry in the posted
@@ -90,7 +96,20 @@ func validateLayoutTiles(tiles []layoutTile) error {
 		return fmt.Errorf("layout has %d tiles; maximum %d", len(tiles), maxTilesPerLayout)
 	}
 	seen := make(map[string]struct{}, len(tiles))
-	for i, t := range tiles {
+	for i := range tiles {
+		t := &tiles[i]
+		// Normalise omitted w/h. GridStack.save() drops fields that
+		// equal their defaults — including w=1 and h=1 — so a 1×N or
+		// N×1 tile arrives with W or H as the Go zero value. Treat
+		// those as the GridStack default of 1 rather than rejecting
+		// the whole payload (the user resizing a tile to 1 col wide
+		// would otherwise silently fail to persist).
+		if t.W == 0 {
+			t.W = 1
+		}
+		if t.H == 0 {
+			t.H = 1
+		}
 		if t.ID == "" {
 			return fmt.Errorf("tile %d: id is empty", i)
 		}
@@ -110,6 +129,17 @@ func validateLayoutTiles(tiles []layoutTile) error {
 		if t.W > maxDim || t.H > maxDim {
 			return fmt.Errorf("tile %q: w/h exceed %d (got w=%d, h=%d)", t.ID, maxDim, t.W, t.H)
 		}
+		// Column-count check: a saved x/w pair that overflows the
+		// 12-column grid can never render as-saved. Without this
+		// check the payload would persist successfully, then load,
+		// then silently get clamped by GridStack — and the user
+		// would see "my layout didn't stick" with no diagnostic.
+		if t.X >= gridColumns {
+			return fmt.Errorf("tile %q: x must be < %d columns (got x=%d)", t.ID, gridColumns, t.X)
+		}
+		if t.X+t.W > gridColumns {
+			return fmt.Errorf("tile %q: x+w exceeds %d columns (got x=%d, w=%d)", t.ID, gridColumns, t.X, t.W)
+		}
 	}
 	return nil
 }

gearbox/internal/framework/handler/api_metrics_layout_test.go

@@ -21,6 +21,28 @@ func TestValidateLayoutTiles(t *testing.T) {
 		t.Errorf("good tiles rejected: %v", err)
 	}
 
+	// GridStack.save() omits w/h when they're at their default of 1,
+	// so a 1×N or N×1 tile arrives with W or H decoded as the Go
+	// zero value. validateLayoutTiles must normalise those to 1 and
+	// accept the tile rather than reject with "w/h must be positive".
+	omitted := []layoutTile{
+		{ID: "card-cpu", X: 0, Y: 0}, // W and H both omitted (=> 0)
+		{ID: "card-mem", X: 1, Y: 0, W: 0, H: 2},
+		{ID: "card-net", X: 2, Y: 0, W: 2, H: 0},
+	}
+	if err := validateLayoutTiles(omitted); err != nil {
+		t.Errorf("tiles with omitted w/h rejected: %v", err)
+	}
+	if omitted[0].W != 1 || omitted[0].H != 1 {
+		t.Errorf("expected omitted w/h normalised to 1, got w=%d h=%d", omitted[0].W, omitted[0].H)
+	}
+	if omitted[1].W != 1 {
+		t.Errorf("expected W=0 normalised to 1, got w=%d", omitted[1].W)
+	}
+	if omitted[2].H != 1 {
+		t.Errorf("expected H=0 normalised to 1, got h=%d", omitted[2].H)
+	}
+
 	cases := []struct {
 		name      string
 		tiles     []layoutTile
@@ -59,11 +81,6 @@ func TestValidateLayoutTiles(t *testing.T) {
 			tiles:     []layoutTile{{ID: "card-cpu", X: maxCoord + 1, Y: 0, W: 1, H: 1}},
 			wantMatch: "exceed",
 		},
-		{
-			name:      "zero width",
-			tiles:     []layoutTile{{ID: "card-cpu", X: 0, Y: 0, W: 0, H: 1}},
-			wantMatch: "w/h must be positive",
-		},
 		{
 			name:      "negative height",
 			tiles:     []layoutTile{{ID: "card-cpu", X: 0, Y: 0, W: 1, H: -1}},
@@ -74,6 +91,16 @@ func TestValidateLayoutTiles(t *testing.T) {
 			tiles:     []layoutTile{{ID: "card-cpu", X: 0, Y: 0, W: maxDim + 1, H: 1}},
 			wantMatch: "w/h exceed",
 		},
+		{
+			name:      "x at column boundary",
+			tiles:     []layoutTile{{ID: "card-cpu", X: gridColumns, Y: 0, W: 1, H: 1}},
+			wantMatch: "x must be <",
+		},
+		{
+			name:      "x+w overflows columns",
+			tiles:     []layoutTile{{ID: "card-cpu", X: 8, Y: 0, W: 6, H: 1}},
+			wantMatch: "x+w exceeds",
+		},
 		{
 			name:      "too many tiles",
 			tiles:     buildOversizedTiles(),

gearbox/internal/framework/templates/pages/metrics.templ

@@ -300,7 +300,7 @@ templ Metrics(user *models.User, servers []models.BoxConfig) {
 				     When a source becomes Available later, makeWidget()
 				     puts the tile back at its default position. -->
 				<div class="grid-stack-item" gs-id="card-nginx" gs-x="0" gs-y="16" gs-w="6" gs-h="4">
-					<div id="card-nginx" data-source="nginx" data-source-card="true" class="grid-stack-item-content chart-card bg-gray-50 dark:bg-slate-700 rounded-lg p-4 relative hidden">
+					<div id="card-nginx" data-source="nginx" data-source-card="true" data-cap-hidden="true" class="grid-stack-item-content chart-card bg-gray-50 dark:bg-slate-700 rounded-lg p-4 relative hidden">
 						<div class="flex justify-between items-center mb-2">
 							<h4 class="text-lg font-medium text-gray-700 dark:text-gray-300"><span class="source-tag">nginx:</span> Connections &amp; Requests</h4>
 							<button onclick="toggleFullscreen('card-nginx')" class="fullscreen-btn p-1 hover:bg-gray-200 dark:hover:bg-slate-600 rounded transition-colors" title="Toggle fullscreen">
@@ -315,7 +315,7 @@ templ Metrics(user *models.User, servers []models.BoxConfig) {
 					</div>
 				</div>
 				<div class="grid-stack-item" gs-id="card-apache" gs-x="6" gs-y="16" gs-w="6" gs-h="4">
-					<div id="card-apache" data-source="apache" data-source-card="true" class="grid-stack-item-content chart-card bg-gray-50 dark:bg-slate-700 rounded-lg p-4 relative hidden">
+					<div id="card-apache" data-source="apache" data-source-card="true" data-cap-hidden="true" class="grid-stack-item-content chart-card bg-gray-50 dark:bg-slate-700 rounded-lg p-4 relative hidden">
 						<div class="flex justify-between items-center mb-2">
 							<h4 class="text-lg font-medium text-gray-700 dark:text-gray-300"><span class="source-tag">Apache:</span> Workers &amp; Requests</h4>
 							<button onclick="toggleFullscreen('card-apache')" class="fullscreen-btn p-1 hover:bg-gray-200 dark:hover:bg-slate-600 rounded transition-colors" title="Toggle fullscreen">
@@ -330,7 +330,7 @@ templ Metrics(user *models.User, servers []models.BoxConfig) {
 					</div>
 				</div>
 				<div class="grid-stack-item" gs-id="card-caddy" gs-x="0" gs-y="20" gs-w="6" gs-h="4">
-					<div id="card-caddy" data-source="caddy" data-source-card="true" class="grid-stack-item-content chart-card bg-gray-50 dark:bg-slate-700 rounded-lg p-4 relative hidden">
+					<div id="card-caddy" data-source="caddy" data-source-card="true" data-cap-hidden="true" class="grid-stack-item-content chart-card bg-gray-50 dark:bg-slate-700 rounded-lg p-4 relative hidden">
 						<div class="flex justify-between items-center mb-2">
 							<h4 class="text-lg font-medium text-gray-700 dark:text-gray-300"><span class="source-tag">Caddy:</span> Requests &amp; Errors</h4>
 							<button onclick="toggleFullscreen('card-caddy')" class="fullscreen-btn p-1 hover:bg-gray-200 dark:hover:bg-slate-600 rounded transition-colors" title="Toggle fullscreen">
@@ -345,7 +345,7 @@ templ Metrics(user *models.User, servers []models.BoxConfig) {
 					</div>
 				</div>
 				<div class="grid-stack-item" gs-id="card-traefik" gs-x="6" gs-y="20" gs-w="6" gs-h="4">
-					<div id="card-traefik" data-source="traefik" data-source-card="true" class="grid-stack-item-content chart-card bg-gray-50 dark:bg-slate-700 rounded-lg p-4 relative hidden">
+					<div id="card-traefik" data-source="traefik" data-source-card="true" data-cap-hidden="true" class="grid-stack-item-content chart-card bg-gray-50 dark:bg-slate-700 rounded-lg p-4 relative hidden">
 						<div class="flex justify-between items-center mb-2">
 							<h4 class="text-lg font-medium text-gray-700 dark:text-gray-300"><span class="source-tag">Traefik:</span> Status Class</h4>
 							<button onclick="toggleFullscreen('card-traefik')" class="fullscreen-btn p-1 hover:bg-gray-200 dark:hover:bg-slate-600 rounded transition-colors" title="Toggle fullscreen">
@@ -1086,9 +1086,13 @@ templ Metrics(user *models.User, servers []models.BoxConfig) {
 			return mapping[cardId];
 		}
 
-		// Handle Escape key to exit fullscreen
+		// Handle Escape key to exit fullscreen. preventDefault so the
+		// global Esc handler in shortcut-help.js doesn't also fire on
+		// the same press (it falls through to history.back() when
+		// nothing else consumes Esc).
 		document.addEventListener('keydown', function(e) {
 			if (e.key === 'Escape' && currentFullscreenCard) {
+				e.preventDefault();
 				toggleFullscreen(currentFullscreenCard);
 			}
 		});
@@ -1498,9 +1502,16 @@ templ Metrics(user *models.User, servers []models.BoxConfig) {
 			}
 
 			// Toggle every element tagged data-source="haproxy". Includes
-			// the chart cards plus the Error Insights panel.
+			// the chart cards plus the Error Insights panel. We set
+			// both `.hidden` (for the visual hide) and
+			// `data-cap-hidden` (the dedicated marker metrics-layout.js
+			// reads so it can tell capability-driven hides apart from
+			// fullscreen-driven `.hidden`s on neighbouring cards).
 			document.querySelectorAll('[data-source="haproxy"]').forEach(function(el) {
 				el.classList.toggle('hidden', !haproxyAvailable);
+				if (el.classList.contains('chart-card')) {
+					el.dataset.capHidden = haproxyAvailable ? 'false' : 'true';
+				}
 			});
 
 			// Gate the per-source chart cards on each source's probe
@@ -1516,6 +1527,7 @@ templ Metrics(user *models.User, servers []models.BoxConfig) {
 				window._availableSources[src] = !!available;
 				document.querySelectorAll('[data-source="' + src + '"][data-source-card]').forEach(function(el) {
 					el.classList.toggle('hidden', !available);
+					el.dataset.capHidden = available ? 'false' : 'true';
 				});
 			}
 
@@ -2833,8 +2845,11 @@ templ Metrics(user *models.User, servers []models.BoxConfig) {
 
 		<!-- GridStack vendor JS (issue #103). Same bundle the Home
 		     gear uses. Loaded with defer so it doesn't block initial
-		     paint; the inline metrics-layout.js script below waits
-		     on DOMContentLoaded before initialising. -->
+		     paint; defer also guarantees both scripts run after the
+		     DOM is parsed, in source order — so metrics-layout.js
+		     can rely on GridStack being defined and on its target
+		     #charts-grid element existing the moment its top-level
+		     IIFE executes (no extra DOMContentLoaded gate needed). -->
 		<script src="/static/js/vendor/gridstack/gridstack-all.js" defer></script>
 		<script src="/static/js/metrics-layout.js" defer></script>
 	}

gearbox/static/js/metrics-layout.js

@@ -104,12 +104,32 @@
   // both files sees the same idiom.
   let suppressChange = true;
 
+  // capHiddenSnapshots remembers the (x, y, w, h) of each tile we
+  // pulled out of the grid because its source went unavailable. When
+  // captureLayout() serialises the grid, those tiles aren't in the
+  // engine (so gs.save() omits them) — but we still want their last-
+  // known positions to ride along in the persisted blob so that a
+  // capability flip back to Available a week later restores the
+  // user's chosen position instead of dropping the tile at its templ
+  // default coords.
+  const capHiddenSnapshots = {};
+
   /** captureLayout returns the current grid state in the same shape
    *  the PATCH endpoint expects — array of {id, x, y, w, h}. We use
    *  gs.save(false) (no node data, just positions); the dashboard
-   *  doesn't need the full GridStack node objects. */
+   *  doesn't need the full GridStack node objects. Capability-hidden
+   *  tiles aren't in the engine so we merge in their snapshots
+   *  before returning, otherwise the saved blob loses them. */
   function captureLayout() {
-    return gs.save(false);
+    const out = gs.save(false) || [];
+    const present = new Set(out.map(function (t) { return t.id; }));
+    Object.keys(capHiddenSnapshots).forEach(function (id) {
+      if (!present.has(id)) {
+        const s = capHiddenSnapshots[id];
+        out.push({ id: id, x: s.x, y: s.y, w: s.w, h: s.h });
+      }
+    });
+    return out;
   }
 
   /** updateContainerHeight pins the grid's min-height to the bottom
@@ -123,9 +143,10 @@
    *
    *  Math: GridStack positions each tile at top = y*cellHeight + margin/2
    *  with height = h*cellHeight - margin. Bottom edge of the last
-   *  row = (y+h)*cellHeight. Adding the margin once at the end
-   *  matches GridStack's own positioning so the spacing below the
-   *  last row equals the spacing between rows. */
+   *  row at index (y+h) sits at (y+h)*cellHeight. We use that bottom
+   *  edge as min-height directly — GridStack already paints the
+   *  marginBottom inside the cellHeight allowance, so adding it
+   *  again would over-pad the section below the grid. */
   function updateContainerHeight() {
     // CAREFUL: GridStack's cellHeight() — note the parens — is an
     // implicit setter when called as a getter. It re-computes
@@ -166,16 +187,25 @@
       const card = item.querySelector(`#${id}`);
       if (!card) return;
 
-      // The inner card carries `.hidden` when capability gating
-      // says the source isn't Available. Pull the surrounding
-      // grid-stack-item out of the engine to free its slot.
-      const isCapHidden = card.classList.contains("hidden");
+      // The capability marker is a dedicated `data-cap-hidden`
+      // attribute set by applyCapabilities() in metrics.templ — we
+      // intentionally do NOT key off `.hidden`, because chart-
+      // fullscreen.js also adds `.hidden` to every non-focused card
+      // while one is fullscreened. Reading `.hidden` here would
+      // remove the entire grid mid-fullscreen and never re-add it.
+      const isCapHidden = card.dataset.capHidden === "true";
       const isInGrid = !!item.gridstackNode;
 
       if (isCapHidden && isInGrid) {
+        // Snapshot the position before removing so captureLayout()
+        // can persist it. Without this the user's chosen position
+        // is lost the moment a source goes unavailable.
+        const n = item.gridstackNode;
+        capHiddenSnapshots[id] = { x: n.x || 0, y: n.y || 0, w: n.w || 1, h: n.h || 1 };
         gs.removeWidget(item, false); // keep DOM, drop slot
       } else if (!isCapHidden && !isInGrid) {
         gs.makeWidget(item);
+        delete capHiddenSnapshots[id];
       }
     });
     updateContainerHeight();
@@ -197,7 +227,13 @@
       if (res.status === 204 || !res.ok) return; // use template defaults
       const layout = await res.json();
       if (!Array.isArray(layout) || layout.length === 0) return;
-      gs.load(layout);
+      // addRemove:false — without it, gs.load() would yank any
+      // grid-stack-item DOM nodes whose gs-id isn't in the loaded
+      // array (capability-hidden tiles that were captured into
+      // capHiddenSnapshots but not currently in the engine). We
+      // intentionally keep those DOM nodes around so makeWidget()
+      // can re-attach them later when their source goes Available.
+      gs.load(layout, false);
     } catch (err) {
       console.debug("metrics layout load failed; using defaults", err);
     }
@@ -221,11 +257,22 @@
     const serverID = getServerID();
     if (!serverID) return;
     try {
-      await fetch(`/api/${serverID}/metrics/layout`, {
+      const res = await fetch(`/api/${serverID}/metrics/layout`, {
         method: "PATCH",
         headers: { "Content-Type": "application/json" },
         body: JSON.stringify(captureLayout()),
       });
+      // fetch() doesn't throw on HTTP 4xx/5xx — surface non-2xx so a
+      // 401/403/413/500 doesn't silently swallow the user's layout
+      // edit. console.warn keeps it visible in devtools without
+      // popping a dialog mid-edit.
+      if (!res.ok) {
+        console.warn(
+          "metrics layout save returned non-2xx",
+          res.status,
+          res.statusText,
+        );
+      }
     } catch (err) {
       console.warn("metrics layout save failed", err);
     }