From 4cd2f05d338233efe91d162fc6568b78be5419a4 Mon Sep 17 00:00:00 2001 From: mAi Date: Fri, 22 May 2026 15:51:43 +0200 Subject: [PATCH] =?UTF-8?q?fix(dashboard):=20t-paliad-238=20=E2=80=94=20hi?= =?UTF-8?q?dden=20widgets=20render=20at=20proper=20size=20in=20edit=20mode?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Symptom (m, 2026-05-22): "super slim columns which I can move but not resize - and they seem greyed out." Hidden widgets in edit mode were rendering as 1×1 slivers because applyLayout left their inline grid- column empty — placeWidgets skipped non-visible entries entirely, so CSS Grid auto-flowed them into the next free cell at 1/12th width. The greyed-out + no-resize-handle parts were correct UX signalling that the widget is hidden; the slim rendering was the bug. Fix: - placeWidgets() gains a {includeHidden} option. When true, a second pass places hidden widgets after the visible pass — collision-aware + cursor-aware so the hidden tray stacks below the active layout without ever displacing a visible widget. applyLayout() passes includeHidden:true in edit mode. - materializePositions() keeps the default (hidden widgets retain their stored coordinates so un-hiding restores them in place). Server-side recovery (belt-and-braces): - SanitizeForRead now also clamps each widget's W/H/X against the catalog Min/Max + grid bounds on load. Stale rows with W below MinW (or above MaxW, or X+W overflowing the grid) heal on the next /api/me/dashboard-layout GET and the cleaned spec is persisted back. W=0 stays 0 (auto/default sentinel — the placer expands it). - The validator stays strict on write; the read-path sanitiser only exists to recover users who got into a bad state under the old rules. Tests: - bun: 4 new cases in dashboard-grid.test.ts pin includeHidden behaviour (hidden skipped by default, two-pass ordering, multi- hidden, no-overlap invariant). - go: 7 sub-tests in dashboard_layout_spec_test.go cover each SanitizeForRead clamp (MinW, MaxW, grid-width, MaxH, X+W overflow, W=0 sentinel, negative X) plus a round-trip Validate guarantee. --- frontend/src/client/dashboard-grid.test.ts | 59 +++++++++ frontend/src/client/dashboard-grid.ts | 58 ++++++++- frontend/src/client/dashboard.ts | 25 +++- internal/services/dashboard_layout_spec.go | 84 +++++++++++- .../services/dashboard_layout_spec_test.go | 122 ++++++++++++++++++ 5 files changed, 332 insertions(+), 16 deletions(-) diff --git a/frontend/src/client/dashboard-grid.test.ts b/frontend/src/client/dashboard-grid.test.ts index 22c3ab0..f5ed5b0 100644 --- a/frontend/src/client/dashboard-grid.test.ts +++ b/frontend/src/client/dashboard-grid.test.ts @@ -210,6 +210,65 @@ describe("placeWidgets — vertical (multi-row) widgets", () => { }); }); +describe("placeWidgets — includeHidden (edit mode)", () => { + test("hidden widgets are skipped by default", () => { + const out = placeWidgets([ + spec("visible", 0, 0, 6), + spec("hidden", 0, 0, 6, 1, false), + ]); + expect(out.has("visible")).toBe(true); + expect(out.has("hidden")).toBe(false); + }); + + test("includeHidden:true places hidden widgets after visible ones", () => { + // Regression for m/paliad#73 / t-paliad-238: in edit mode hidden + // widgets MUST receive a placement, otherwise applyLayout leaves + // their inline grid-column empty and CSS Grid auto-flows them as + // 1×1 slivers ("super slim greyed-out column"). + const out = placeWidgets([ + spec("active", 0, 0, 12), + spec("hidden", 0, 0, 6, 1, false), + ], { includeHidden: true }); + expect(out.has("hidden")).toBe(true); + const h = out.get("hidden")!; + // Must keep its requested width (6), not collapse to 1. + expect(h.w).toBe(6); + // Must land below the visible widget — never overlap or steal cells. + expect(h.y).toBeGreaterThanOrEqual(1); + expect(hasOverlap(out)).toBeNull(); + }); + + test("includeHidden two-pass: visible widgets keep priority over hidden", () => { + // Hidden widget stored at (0, 0) shouldn't displace a visible + // widget that wants (0, 0). The visible pass runs first, claims + // (0, 0); the hidden widget is then placed wherever free — the + // placer happily fits it next to the visible widget on the same + // row if there's room. The hard invariant is just no-overlap. + const out = placeWidgets([ + spec("active", 0, 0, 6), + spec("hidden-at-origin", 0, 0, 6, 1, false), + ], { includeHidden: true }); + expect(out.get("active")).toEqual({ x: 0, y: 0, w: 6, h: 1 }); + expect(out.has("hidden-at-origin")).toBe(true); + expect(hasOverlap(out)).toBeNull(); + }); + + test("multiple hidden widgets all receive valid placements", () => { + const out = placeWidgets([ + spec("a", 0, 0, 12), + spec("h1", undefined, undefined, 6, 1, false), + spec("h2", undefined, undefined, 6, 1, false), + spec("h3", undefined, undefined, 12, 1, false), + ], { includeHidden: true }); + expect(out.size).toBe(4); + for (const r of out.values()) { + expect(r.w).toBeGreaterThanOrEqual(1); + expect(r.x + r.w).toBeLessThanOrEqual(GRID_COLUMNS); + } + expect(hasOverlap(out)).toBeNull(); + }); +}); + describe("clamp helpers", () => { test("clampW respects min/max bounds", () => { expect(clampW(2, { min_w: 4, max_w: 12 })).toBe(4); diff --git a/frontend/src/client/dashboard-grid.ts b/frontend/src/client/dashboard-grid.ts index 49c88bf..7b88314 100644 --- a/frontend/src/client/dashboard-grid.ts +++ b/frontend/src/client/dashboard-grid.ts @@ -133,10 +133,30 @@ function findFreeSlot( return { x: 0, y: startY + MAX_SCAN_ROWS }; } -// placeWidgets assigns no-overlap grid coordinates to every visible -// widget. Hidden widgets are skipped and contribute no placement. +// PlaceOptions tunes the placer for the caller's render-vs-persist +// needs. +export interface PlaceOptions { + // When true, hidden widgets are placed too — for edit-mode rendering + // where the user can see + un-hide them inline. The two-pass order + // (visible first, then hidden) guarantees hidden widgets never + // displace visible ones: they get whatever cells are left below the + // active layout. Default false matches view-mode behaviour and the + // persistence path (materializePositions) where hidden widgets + // retain their stored coordinates instead of being repacked. + // + // Without this option, hidden widgets in edit mode were left without + // an explicit grid-column inline style by applyLayout(), so CSS Grid + // auto-flowed them into the next free cell at 1×1 — the "super slim + // greyed-out column" symptom of m/paliad#73 / t-paliad-238. + includeHidden?: boolean; +} + +// placeWidgets assigns no-overlap grid coordinates to widgets. By +// default only visible widgets receive placements; pass +// {includeHidden:true} to also place hidden widgets after the visible +// pass (used by applyLayout in edit mode). // -// Algorithm: iterate widgets in input order. For each visible widget: +// Algorithm — per pass: // 1. Clamp w/h against catalog bounds. // 2. If the spec carries explicit x and y, try that slot. On a // collision, search downward starting at the requested y for the @@ -150,8 +170,15 @@ function findFreeSlot( // real-world layout — placing the explicit widgets first would change // the visual order, so we keep input order and let auto-flow widgets // step around any explicit blockers via the same collision search. +// +// Two-pass behaviour for hidden widgets: the visible pass owns its +// own auto-flow cursor; the hidden pass continues from where the +// visible pass left off so the hidden widgets stack right under the +// active layout. The shared Occupancy bitmap guarantees the second +// pass can never overlap a placed visible widget. export function placeWidgets( widgets: WidgetPlacementInput[], + options: PlaceOptions = {}, ): Map { const out = new Map(); const occ = new Occupancy(); @@ -165,8 +192,7 @@ export function placeWidgets( let cursorY = 0; let rowMaxH = 0; - for (const w of widgets) { - if (!w.visible) continue; + const placeOne = (w: WidgetPlacementInput): void => { const dw = clampW(w.w ?? w.bound?.default_w ?? GRID_COLUMNS, w.bound); const dh = clampH(w.h ?? w.bound?.default_h ?? 1, w.bound); @@ -210,6 +236,28 @@ export function placeWidgets( occ.mark(placed.x, placed.y, dw, dh); out.set(w.key, { x: placed.x, y: placed.y, w: dw, h: dh }); + }; + + // Pass 1: visible widgets. They own the active layout. + for (const w of widgets) { + if (!w.visible) continue; + placeOne(w); + } + + // Pass 2: hidden widgets (edit-mode only). Wrap the cursor to the + // start of the next row before the second pass so the hidden tray + // visually separates from the active layout — even if the last + // visible widget left half a row open. + if (options.includeHidden) { + if (cursorX > 0) { + cursorY += rowMaxH || 1; + cursorX = 0; + rowMaxH = 0; + } + for (const w of widgets) { + if (w.visible) continue; + placeOne(w); + } } return out; diff --git a/frontend/src/client/dashboard.ts b/frontend/src/client/dashboard.ts index 844b430..44d200f 100644 --- a/frontend/src/client/dashboard.ts +++ b/frontend/src/client/dashboard.ts @@ -1922,10 +1922,15 @@ function applyLayout(): void { if (k) byKey.set(k, el); }); - // Compute effective placements (with auto-flow fill-in for missing - // y values). The visible widgets are placed deterministically so the - // grid renders identically across reloads. - const placements = computePlacements(currentLayout.widgets); + // Compute effective placements. In edit mode we also include hidden + // widgets so they render at their stored (or default) dimensions + // dimmed-but-visible — without this they'd inherit no inline grid- + // column and CSS Grid would auto-flow them as 1×1 slivers, producing + // the "super slim greyed-out column" symptom (m/paliad#73). In view + // mode hidden widgets are display:none and reserve no cells. + const placements = computePlacements(currentLayout.widgets, { + includeHidden: editMode, + }); for (const w of currentLayout.widgets) { const el = byKey.get(w.key); @@ -1952,7 +1957,15 @@ function applyLayout(): void { // overlap invariant: if two widgets request colliding cells (drag-drop // swap with mismatched widths, resize-grow into a sibling, etc.) the // later one is shifted down to the next free row. See m/paliad#70. -function computePlacements(widgets: DashboardWidgetRef[]): Map { +// +// includeHidden=true is used by applyLayout in edit mode to also place +// hidden widgets after the visible pass — so the hidden tray renders +// at proper size below the active layout. Default (false) matches the +// persistence + render paths where hidden widgets carry no placement. +function computePlacements( + widgets: DashboardWidgetRef[], + options: { includeHidden?: boolean } = {}, +): Map { const inputs: WidgetPlacementInput[] = widgets.map((w) => ({ key: w.key, visible: w.visible, @@ -1962,7 +1975,7 @@ function computePlacements(widgets: DashboardWidgetRef[]): Map DashboardGridColumns { + w.W = DashboardGridColumns + changed = true + } + // W == 0 is the "auto / default" sentinel — leave it untouched so + // downstream renderers can substitute DefaultW. Only clamp non-zero + // values against the per-widget Min/Max. + if w.W > 0 { + if def.MinW > 0 && w.W < def.MinW { + w.W = def.MinW + changed = true + } + if def.MaxW > 0 && w.W > def.MaxW { + w.W = def.MaxW + changed = true + } + } + + if w.H < 0 { + w.H = 0 + changed = true + } + if w.H > MaxGridRowSpan { + w.H = MaxGridRowSpan + changed = true + } + if w.H > 0 { + if def.MinH > 0 && w.H < def.MinH { + w.H = def.MinH + changed = true + } + if def.MaxH > 0 && w.H > def.MaxH { + w.H = def.MaxH + changed = true + } + } + + if w.X < 0 { + w.X = 0 + changed = true + } + if w.X >= DashboardGridColumns { + w.X = DashboardGridColumns - 1 + changed = true + } + if w.W > 0 && w.X+w.W > DashboardGridColumns { + w.X = DashboardGridColumns - w.W + changed = true + } + if w.Y < 0 { + w.Y = 0 + changed = true + } + + return changed +} + // ParseDashboardLayoutSpec decodes JSON bytes and validates. Used by the // HTTP handler on incoming request bodies. func ParseDashboardLayoutSpec(b []byte) (DashboardLayoutSpec, error) { diff --git a/internal/services/dashboard_layout_spec_test.go b/internal/services/dashboard_layout_spec_test.go index 06bff3b..ac45c00 100644 --- a/internal/services/dashboard_layout_spec_test.go +++ b/internal/services/dashboard_layout_spec_test.go @@ -279,6 +279,128 @@ func TestDashboardLayoutSpec_SanitizeForRead_DropsUnknownKeys(t *testing.T) { } } +// TestDashboardLayoutSpec_SanitizeForRead_ClampsOutOfRange covers the +// m/paliad#73 recovery path: a stale row in user_dashboard_layouts +// carrying a W below MinW (or above MaxW) must be normalised on load so +// the user doesn't get stranded with super-slim columns. Pre-fix the +// sanitizer only dropped unknown keys; sizes passed through verbatim. +func TestDashboardLayoutSpec_SanitizeForRead_ClampsOutOfRange(t *testing.T) { + // upcoming-deadlines: MinW=4, MaxW=12, MinH=1, MaxH=4 (per catalog). + def, ok := LookupWidgetDef(WidgetUpcomingDeadlines) + if !ok { + t.Fatal("LookupWidgetDef(WidgetUpcomingDeadlines) = !ok") + } + cases := []struct { + name string + in DashboardWidgetRef + wantW int + wantH int + wantX int + wantY int + wantOK bool // expected SanitizeForRead-returns-true + }{ + { + name: "W below MinW snaps to MinW", + in: DashboardWidgetRef{Key: WidgetUpcomingDeadlines, Visible: true, X: 0, Y: 0, W: 1, H: 1}, + wantW: def.MinW, + wantH: 1, + wantOK: true, + }, + { + name: "W above MaxW snaps to MaxW", + in: DashboardWidgetRef{Key: WidgetUpcomingDeadlines, Visible: true, X: 0, Y: 0, W: 99, H: 1}, + wantW: DashboardGridColumns, + wantH: 1, + wantOK: true, + }, + { + name: "W above grid width snaps to grid width", + in: DashboardWidgetRef{Key: WidgetUpcomingDeadlines, Visible: true, X: 0, Y: 0, W: 50, H: 1}, + wantW: DashboardGridColumns, + wantH: 1, + wantOK: true, + }, + { + name: "H above MaxGridRowSpan snaps to MaxGridRowSpan", + in: DashboardWidgetRef{Key: WidgetUpcomingDeadlines, Visible: true, X: 0, Y: 0, W: 6, H: 99}, + wantW: 6, + wantH: def.MaxH, // upcoming-deadlines MaxH=4 < MaxGridRowSpan=5 + wantOK: true, + }, + { + name: "X+W overflowing grid snaps X down", + in: DashboardWidgetRef{Key: WidgetUpcomingDeadlines, Visible: true, X: 10, Y: 0, W: 6, H: 1}, + wantW: 6, + wantH: 1, + wantX: 6, // 12 - 6 = 6 + wantOK: true, + }, + { + name: "W=0 stays 0 (auto / default sentinel)", + in: DashboardWidgetRef{Key: WidgetUpcomingDeadlines, Visible: true, X: 0, Y: 0, W: 0, H: 0}, + wantW: 0, + wantH: 0, + wantOK: false, + }, + { + name: "negative X snaps to 0", + in: DashboardWidgetRef{Key: WidgetUpcomingDeadlines, Visible: true, X: -3, Y: 0, W: 6, H: 1}, + wantW: 6, + wantH: 1, + wantX: 0, + wantOK: true, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + s := DashboardLayoutSpec{Version: LayoutSpecVersion, Widgets: []DashboardWidgetRef{tc.in}} + changed := s.SanitizeForRead() + if changed != tc.wantOK { + t.Errorf("SanitizeForRead returned %v; want %v", changed, tc.wantOK) + } + if len(s.Widgets) != 1 { + t.Fatalf("expected 1 widget after sanitize, got %d", len(s.Widgets)) + } + got := s.Widgets[0] + if got.W != tc.wantW { + t.Errorf("W = %d; want %d", got.W, tc.wantW) + } + if got.H != tc.wantH { + t.Errorf("H = %d; want %d", got.H, tc.wantH) + } + if got.X != tc.wantX { + t.Errorf("X = %d; want %d", got.X, tc.wantX) + } + if got.Y != tc.wantY { + t.Errorf("Y = %d; want %d", got.Y, tc.wantY) + } + }) + } +} + +// TestDashboardLayoutSpec_SanitizeForRead_ClampedSpecPassesValidate is +// the round-trip guarantee — after the sanitiser heals a stale row, the +// result must be acceptable to Validate so the next PUT doesn't reject +// the user's layout. Without this guarantee, sanitizing on read could +// produce a layout the validator won't accept on the autosave path. +func TestDashboardLayoutSpec_SanitizeForRead_ClampedSpecPassesValidate(t *testing.T) { + s := DashboardLayoutSpec{ + Version: LayoutSpecVersion, + Widgets: []DashboardWidgetRef{ + {Key: WidgetUpcomingDeadlines, Visible: true, X: 0, Y: 0, W: 1, H: 1}, + {Key: WidgetUpcomingDeadlines, Visible: true, X: 50, Y: 0, W: 99, H: 99}, // duplicate key — Validate will reject; this case checks size clamp at least + }, + } + // Trim to one widget for the validate assertion (duplicates are a + // separate concern). + s.Widgets = s.Widgets[:1] + s.SanitizeForRead() + if err := s.Validate(); err != nil { + t.Errorf("Validate after SanitizeForRead returned %v; want nil", err) + } +} + func TestDashboardLayoutSpec_SanitizeForRead_NoopOnClean(t *testing.T) { s := FactoryDefaultLayout() if s.SanitizeForRead() {