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

sarg3nt · claude · sarg3nt · commit fc023d74a1d2 · 2026-05-14T23:33:55.000-07:00
JS:
- metrics-layout.js: getServerID was missing the single-server
  fallback (#default-server-id) that metrics.templ's inline helper
  consults. Single-box installs silently no-op'd every layout
  GET/PATCH/DELETE because serverID came back empty. Now matches
  the inline helper byte-for-byte: select → default-server-id →
  URL.
- metrics-layout.js: saveLayout now takes a {flush: true} option
  used by the edit-mode exit handler. Without flush, a tab-close
  right after "Done editing" could drop the layout because the
  debounce was still pending. Drag/resize calls still use the
  debounced path so a multi-tile drag flushes once, not N times.
- metrics-layout.js: resetLayout now cancels any pending debounced
  PATCH AND suppresses further saves before issuing the DELETE.
  Without this a fast drag-then-reset race could land a PATCH
  after the DELETE and resurrect the row the user just threw away.
- metrics-layout.js: GridStack margin 8 → 16. Comment had claimed
  16 (matches Tailwind gap-4 and Home's gridstack init); value
  was the typo. Matched to Home now so the visual idiom is
  consistent across pages.

Handler:
- api_metrics_layout.go: PATCH now runs validateLayoutTiles before
  persisting. Previously a malformed client could land tiles with
  empty IDs, negative coords, zero/negative dimensions, or
  duplicate IDs that would silently corrupt the saved blob. New
  validator enforces: non-empty id, x/y in [0, maxCoord], w/h in
  [1, maxDim], unique ids across the array, tile count in [1,
  maxTilesPerLayout=64]. First failure returned with an actionable
  message (one good error beats a list when the source is a JS
  caller, not a hand-edited file).
- api_metrics_layout_test.go (new): table-driven coverage of
  every validateLayoutTiles rejection path plus the happy path.
- metrics_layouts_test.go (new): DB round-trip coverage for
  GetMetricsLayout (empty → ErrNoMetricsLayout sentinel), Save
  upsert behaviour, isolation across (user, box) pairs, Delete
  resets-to-default flow, Delete no-op on absent row.

Co-Authored-By: Claude Opus 4.7 (1M context) &lt;noreply@anthropic.com&gt;
diff --git a/gearbox/internal/framework/database/metrics_layouts_test.go b/gearbox/internal/framework/database/metrics_layouts_test.go
@@ -0,0 +1,141 @@
+package database
+
+import (
+	"bytes"
+	"errors"
+	"testing"
+)
+
+func TestGetMetricsLayoutMissingReturnsSentinel(t *testing.T) {
+	// First read on an empty table must return ErrNoMetricsLayout so
+	// the handler can translate to 204 No Content — anything else
+	// would mask the "no saved layout, use template defaults" path.
+	db := setupTestDB(t)
+
+	_, err := db.GetMetricsLayout("user-1", "box-1")
+	if !errors.Is(err, ErrNoMetricsLayout) {
+		t.Errorf("expected ErrNoMetricsLayout, got %v", err)
+	}
+}
+
+func TestSaveAndGetMetricsLayout(t *testing.T) {
+	// Round-trip the JSON blob byte-for-byte — the storage layer is
+	// agnostic to GridStack's payload shape, so the bytes coming back
+	// out must match the bytes going in.
+	db := setupTestDB(t)
+
+	payload := []byte(`[{"id":"card-cpu","x":0,"y":0,"w":6,"h":4},{"id":"card-memory","x":6,"y":0,"w":6,"h":4}]`)
+	if err := db.SaveMetricsLayout("user-1", "box-1", payload); err != nil {
+		t.Fatalf("save: %v", err)
+	}
+
+	got, err := db.GetMetricsLayout("user-1", "box-1")
+	if err != nil {
+		t.Fatalf("get: %v", err)
+	}
+	if !bytes.Equal(got.Layout, payload) {
+		t.Errorf("layout round-trip = %s, want %s", got.Layout, payload)
+	}
+	if got.UserID != "user-1" || got.ServerID != "box-1" {
+		t.Errorf("metadata wrong: %+v", got)
+	}
+	if got.UpdatedAt.IsZero() {
+		t.Error("UpdatedAt should be set by SaveMetricsLayout")
+	}
+}
+
+func TestSaveMetricsLayoutUpserts(t *testing.T) {
+	// PK is (user_id, server_id); a second save for the same pair
+	// must replace the layout, not error or create a second row.
+	db := setupTestDB(t)
+
+	first := []byte(`[{"id":"card-cpu","x":0,"y":0,"w":6,"h":4}]`)
+	second := []byte(`[{"id":"card-cpu","x":6,"y":0,"w":6,"h":4}]`)
+
+	if err := db.SaveMetricsLayout("u", "b", first); err != nil {
+		t.Fatalf("first save: %v", err)
+	}
+	if err := db.SaveMetricsLayout("u", "b", second); err != nil {
+		t.Fatalf("second save: %v", err)
+	}
+	got, err := db.GetMetricsLayout("u", "b")
+	if err != nil {
+		t.Fatalf("get: %v", err)
+	}
+	if !bytes.Equal(got.Layout, second) {
+		t.Errorf("expected second save to win, got %s", got.Layout)
+	}
+}
+
+func TestMetricsLayoutIsolatedPerUserAndBox(t *testing.T) {
+	// (user_id, server_id) is the PK — different users on the same
+	// box, or the same user on different boxes, must keep separate
+	// layouts.
+	db := setupTestDB(t)
+
+	aliceBox1 := []byte(`[{"id":"card-cpu","x":1,"y":0,"w":6,"h":4}]`)
+	bobBox1 := []byte(`[{"id":"card-cpu","x":2,"y":0,"w":6,"h":4}]`)
+	aliceBox2 := []byte(`[{"id":"card-cpu","x":3,"y":0,"w":6,"h":4}]`)
+
+	if err := db.SaveMetricsLayout("alice", "box-1", aliceBox1); err != nil {
+		t.Fatal(err)
+	}
+	if err := db.SaveMetricsLayout("bob", "box-1", bobBox1); err != nil {
+		t.Fatal(err)
+	}
+	if err := db.SaveMetricsLayout("alice", "box-2", aliceBox2); err != nil {
+		t.Fatal(err)
+	}
+
+	cases := []struct {
+		user, box string
+		want      []byte
+	}{
+		{"alice", "box-1", aliceBox1},
+		{"bob", "box-1", bobBox1},
+		{"alice", "box-2", aliceBox2},
+	}
+	for _, tc := range cases {
+		got, err := db.GetMetricsLayout(tc.user, tc.box)
+		if err != nil {
+			t.Errorf("(%s,%s) get: %v", tc.user, tc.box, err)
+			continue
+		}
+		if !bytes.Equal(got.Layout, tc.want) {
+			t.Errorf("(%s,%s) layout = %s, want %s", tc.user, tc.box, got.Layout, tc.want)
+		}
+	}
+}
+
+func TestDeleteMetricsLayoutResetsToDefault(t *testing.T) {
+	// Delete must drop the row so the subsequent Get returns
+	// ErrNoMetricsLayout — that's what drives the "fall back to
+	// template defaults" path after the operator hits Reset.
+	db := setupTestDB(t)
+
+	payload := []byte(`[{"id":"card-cpu","x":0,"y":0,"w":6,"h":4}]`)
+	if err := db.SaveMetricsLayout("u", "b", payload); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := db.GetMetricsLayout("u", "b"); err != nil {
+		t.Fatalf("pre-delete get: %v", err)
+	}
+	if err := db.DeleteMetricsLayout("u", "b"); err != nil {
+		t.Fatalf("delete: %v", err)
+	}
+	_, err := db.GetMetricsLayout("u", "b")
+	if !errors.Is(err, ErrNoMetricsLayout) {
+		t.Errorf("expected ErrNoMetricsLayout after delete, got %v", err)
+	}
+}
+
+func TestDeleteMetricsLayoutIsNoOpWhenAbsent(t *testing.T) {
+	// Reset on a box with no saved layout shouldn't surface as an
+	// error — the user clicked the button; the desired state is
+	// "no row", which is already true.
+	db := setupTestDB(t)
+
+	if err := db.DeleteMetricsLayout("u", "never-saved"); err != nil {
+		t.Errorf("delete of absent row should be no-op, got %v", err)
+	}
+}
diff --git a/gearbox/internal/framework/handler/api_metrics_layout.go b/gearbox/internal/framework/handler/api_metrics_layout.go
@@ -25,6 +25,7 @@ package handler
 import (
 	"encoding/json"
 	"errors"
+	"fmt"
 	"io"
 	"net/http"
 
@@ -40,14 +41,30 @@ import (
 // dashboard PATCH endpoints accept.
 const maxLayoutBytes = 16 * 1024
 
-// layoutTile is the minimum shape we require each entry in the
-// posted layout array to carry. GridStack's `save()` includes
-// these four fields for every node; the `id` is the stable DOM id
-// of the tile (e.g. "card-cpu"). We don't enforce the id's value
-// against the known set of cards — the dashboard renders cards by
-// id and ignores anything it doesn't recognise, so an unknown id
-// in the saved layout is a no-op at render time rather than a
-// failure mode worth rejecting here.
+// maxTilesPerLayout caps the per-PATCH tile count. The page has 11
+// known cards today (7 baseline + 4 per-source); 64 is a comfortable
+// ceiling that survives future growth without letting a misbehaving
+// client commit a thousand-tile blob.
+const maxTilesPerLayout = 64
+
+// maxCoord / maxDim bound the GridStack coordinate space. The grid
+// renders at 12 columns wide; the metrics page typically reaches
+// y ~ 24 on the default layout. Caps a couple of orders of magnitude
+// higher so a wide future layout still fits while a "garbage value"
+// like `x: 9_999_999` gets rejected.
+const (
+	maxCoord = 1000
+	maxDim   = 100
+)
+
+// layoutTile is the shape we require each entry in the posted
+// layout array to carry. GridStack's `save()` includes these four
+// fields for every node; the `id` is the stable DOM id of the tile
+// (e.g. "card-cpu"). We don't enforce the id's value against the
+// known set of cards — the dashboard renders cards by id and
+// ignores anything it doesn't recognise, so an unknown id in the
+// saved layout is a no-op at render time rather than a failure
+// mode worth rejecting here.
 type layoutTile struct {
 	ID string `json:"id"`
 	X  int    `json:"x"`
@@ -56,6 +73,47 @@ type layoutTile struct {
 	H  int    `json:"h"`
 }
 
+// validateLayoutTiles enforces the per-tile invariants we need to
+// trust the stored blob on read-back: non-empty IDs, non-negative
+// coordinates, positive dimensions, bounded values, unique IDs.
+// Without this a misbehaving client could persist tiles with
+// negative coords / zero dimensions / duplicate IDs that would
+// surface as confusing render bugs later. Returns the first
+// problem found rather than aggregating — one good error is more
+// actionable than a list when the source is a misbehaving JS
+// caller, not a hand-edited file.
+func validateLayoutTiles(tiles []layoutTile) error {
+	if len(tiles) == 0 {
+		return errors.New("layout must contain at least one tile")
+	}
+	if len(tiles) > maxTilesPerLayout {
+		return fmt.Errorf("layout has %d tiles; maximum %d", len(tiles), maxTilesPerLayout)
+	}
+	seen := make(map[string]struct{}, len(tiles))
+	for i, t := range tiles {
+		if t.ID == "" {
+			return fmt.Errorf("tile %d: id is empty", i)
+		}
+		if _, dup := seen[t.ID]; dup {
+			return fmt.Errorf("tile %d: duplicate id %q", i, t.ID)
+		}
+		seen[t.ID] = struct{}{}
+		if t.X < 0 || t.Y < 0 {
+			return fmt.Errorf("tile %q: x/y must be non-negative (got x=%d, y=%d)", t.ID, t.X, t.Y)
+		}
+		if t.X > maxCoord || t.Y > maxCoord {
+			return fmt.Errorf("tile %q: x/y exceed %d (got x=%d, y=%d)", t.ID, maxCoord, t.X, t.Y)
+		}
+		if t.W <= 0 || t.H <= 0 {
+			return fmt.Errorf("tile %q: w/h must be positive (got w=%d, h=%d)", t.ID, t.W, t.H)
+		}
+		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)
+		}
+	}
+	return nil
+}
+
 // APIMetricsLayoutGetHandler returns the user's saved metrics
 // layout for one box. Returns 204 No Content when nothing's saved
 // — that's the signal for the front-end to use the template's
@@ -136,8 +194,8 @@ func (h *Handler) APIMetricsLayoutPatchHandler(w http.ResponseWriter, r *http.Re
 		http.Error(w, "Invalid layout JSON: "+err.Error(), http.StatusBadRequest)
 		return
 	}
-	if len(tiles) == 0 {
-		http.Error(w, "Layout must contain at least one tile", http.StatusBadRequest)
+	if err := validateLayoutTiles(tiles); err != nil {
+		http.Error(w, "Invalid layout: "+err.Error(), http.StatusBadRequest)
 		return
 	}
 
diff --git a/gearbox/internal/framework/handler/api_metrics_layout_test.go b/gearbox/internal/framework/handler/api_metrics_layout_test.go
@@ -0,0 +1,125 @@
+package handler
+
+import (
+	"strings"
+	"testing"
+)
+
+// validateLayoutTiles is a pure function — these tests cover every
+// rejection path the PATCH handler relies on (issue #103 review on
+// PR #104). Full HTTP-level handler tests would need a wired Handler
+// with db + authManager; for the validation surface specifically the
+// table tests below match the handler's behaviour 1:1 because the
+// handler calls validateLayoutTiles directly and forwards its error
+// message verbatim.
+func TestValidateLayoutTiles(t *testing.T) {
+	good := []layoutTile{
+		{ID: "card-cpu", X: 0, Y: 0, W: 6, H: 4},
+		{ID: "card-memory", X: 6, Y: 0, W: 6, H: 4},
+	}
+	if err := validateLayoutTiles(good); err != nil {
+		t.Errorf("good tiles rejected: %v", err)
+	}
+
+	cases := []struct {
+		name      string
+		tiles     []layoutTile
+		wantMatch string
+	}{
+		{
+			name:      "empty array",
+			tiles:     nil,
+			wantMatch: "at least one tile",
+		},
+		{
+			name:      "empty id",
+			tiles:     []layoutTile{{ID: "", X: 0, Y: 0, W: 1, H: 1}},
+			wantMatch: "id is empty",
+		},
+		{
+			name: "duplicate id",
+			tiles: []layoutTile{
+				{ID: "card-cpu", X: 0, Y: 0, W: 6, H: 4},
+				{ID: "card-cpu", X: 6, Y: 0, W: 6, H: 4},
+			},
+			wantMatch: "duplicate id",
+		},
+		{
+			name:      "negative x",
+			tiles:     []layoutTile{{ID: "card-cpu", X: -1, Y: 0, W: 1, H: 1}},
+			wantMatch: "must be non-negative",
+		},
+		{
+			name:      "negative y",
+			tiles:     []layoutTile{{ID: "card-cpu", X: 0, Y: -5, W: 1, H: 1}},
+			wantMatch: "must be non-negative",
+		},
+		{
+			name:      "x exceeds bound",
+			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}},
+			wantMatch: "w/h must be positive",
+		},
+		{
+			name:      "w exceeds bound",
+			tiles:     []layoutTile{{ID: "card-cpu", X: 0, Y: 0, W: maxDim + 1, H: 1}},
+			wantMatch: "w/h exceed",
+		},
+		{
+			name:      "too many tiles",
+			tiles:     buildOversizedTiles(),
+			wantMatch: "maximum",
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			err := validateLayoutTiles(tc.tiles)
+			if err == nil {
+				t.Fatalf("expected validation error containing %q, got nil", tc.wantMatch)
+			}
+			if !strings.Contains(err.Error(), tc.wantMatch) {
+				t.Errorf("error %q does not contain %q", err.Error(), tc.wantMatch)
+			}
+		})
+	}
+}
+
+// buildOversizedTiles returns one more tile than the cap so the
+// "maximum" rejection branch fires.
+func buildOversizedTiles() []layoutTile {
+	tiles := make([]layoutTile, maxTilesPerLayout+1)
+	for i := range tiles {
+		tiles[i] = layoutTile{
+			ID: pseudoTileID(i),
+			X:  0,
+			Y:  i * 4,
+			W:  1,
+			H:  1,
+		}
+	}
+	return tiles
+}
+
+// pseudoTileID returns a deterministic unique-ish id for the
+// over-cap test. Using a simple letter cycle avoids the duplicate-
+// id rejection firing first (which would shadow the cap check).
+func pseudoTileID(i int) string {
+	const alphabet = "abcdefghijklmnopqrstuvwxyz0123456789"
+	if i < len(alphabet) {
+		return "card-" + string(alphabet[i])
+	}
+	// Two-char IDs for indices past 36; up to 36*36 = 1296 unique
+	// values which more than covers maxTilesPerLayout+1.
+	a := alphabet[i/len(alphabet)]
+	b := alphabet[i%len(alphabet)]
+	return "card-" + string([]byte{a, b})
+}
diff --git a/gearbox/static/js/metrics-layout.js b/gearbox/static/js/metrics-layout.js
