fix(client): stop tab changes corrupting and scrambling widget layouts

client/src/components/WidgetContainer.jsx

@@ -1,4 +1,4 @@
-import React, { Suspense, useState, useEffect, useRef, useCallback } from 'react';
+import React, { Suspense, useState, useEffect, useMemo, useRef, useCallback } from 'react';
 import { Box, IconButton } from '@mui/material';
 import GridLayout, { getCompactor } from 'react-grid-layout';
 import 'react-grid-layout/css/styles.css';
@@ -12,6 +12,8 @@ import {
   scaleLayoutItem,
 } from '../utils/gridLayout.js';
 import CountdownCircle from './CountdownCircle';
+import { shouldAcceptLayoutChange } from '../utils/layoutSync';
+import { buildLayout } from '../utils/gridPlacement';
 
 // No auto-compaction; block overlaps (same as compactType={null} + preventCollision).
 const GRID_COMPACTOR = getCompactor(null, false, true);
@@ -64,6 +66,10 @@ const WidgetContainer = ({
   const prevLockedRef = useRef(locked);
   const hasInitializedLockEffectRef = useRef(false);
   const saveTimerRef = useRef(null);
+  // Which tab the current `layout` state was built for. Null until the first
+  // rebuild lands. Guards against saving a layout mid tab change — see
+  // shouldAcceptLayoutChange.
+  const layoutTabRef = useRef(null);
   const resizeTapGuardRef = useRef(new Map());
 
   const saveLayoutsToApi = useCallback((layoutItems, tabNumber, cols) => {
@@ -137,85 +143,18 @@ const WidgetContainer = ({
     if (widgetsChanged) {
       prevWidgetIdsRef.current = currentCacheKey;
 
-      const cols = gridCols;
-      const placed = [];
-
-      const collides = (x, y, w, h) => {
-        return placed.some(p =>
-          x < p.x + p.w && x + w > p.x && y < p.y + p.h && y + h > p.y
-        );
-      };
-
-      const findFreePosition = (w, h) => {
-        for (let row = 0; row < 200; row++) {
-          for (let col = 0; col <= cols - w; col++) {
-            if (!collides(col, row, w, h)) return { x: col, y: row };
-          }
-        }
-        return { x: 0, y: 0 };
-      };
-
-      const initialLayout = widgets.map((widget) => {
-        const minW = widget.minWidth || 3;
-        const minH = widget.minHeight || 2;
-        let item;
-
-        if (widget.savedLayout) {
-          const scaled = layoutItemFromNormalized(
-            {
-              x: widget.savedLayout.x ?? widget.defaultPosition.x,
-              y: widget.savedLayout.y ?? widget.defaultPosition.y,
-              w: widget.savedLayout.w || widget.defaultSize.width,
-              h: widget.savedLayout.h || widget.defaultSize.height,
-              minW,
-              minH,
-            },
-            cols
-          );
-          item = {
-            i: widget.id,
-            ...scaled,
-            static: lockedRef.current,
-          };
-        } else {
-          const scaledDefault = layoutItemFromNormalized(
-            {
-              x: widget.defaultPosition.x,
-              y: widget.defaultPosition.y,
-              w: widget.defaultSize.width,
-              h: widget.defaultSize.height,
-              minW,
-              minH,
-            },
-            cols
-          );
-          const pos = findFreePosition(scaledDefault.w, scaledDefault.h);
-          item = {
-            i: widget.id,
-            x: pos.x,
-            y: pos.y,
-            w: scaledDefault.w,
-            h: scaledDefault.h,
-            minW: scaledDefault.minW,
-            minH: scaledDefault.minH,
-            static: lockedRef.current,
-          };
-        }
-
-        placed.push({ x: item.x, y: item.y, w: item.w, h: item.h });
-        return item;
-      });
+      const initialLayout = buildLayout(widgets, gridCols, lockedRef.current);
       setLayout(initialLayout);
+      layoutTabRef.current = activeTab;
     } else if (colsChanged) {
       setLayout((currentLayout) => {
         const nextLayout = currentLayout.map((item) => ({
           ...scaleLayoutItem(item, prevCols, gridCols),
           static: lockedRef.current,
         }));
-        const calendarBefore = currentLayout.find((item) => item.i === 'calendar-widget');
-        const calendarAfter = nextLayout.find((item) => item.i === 'calendar-widget');
         return nextLayout;
       });
+      layoutTabRef.current = activeTab;
     }
 
     prevGridColsRef.current = gridCols;
@@ -233,7 +172,14 @@ const WidgetContainer = ({
         static: locked
       }));
 
-      const shouldPersistLockedLayouts = hasInitializedLockEffectRef.current && !wasLocked && locked;
+      // Same invariant as handleLayoutChange: only persist when the layout state
+      // and the active tab agree. Locking mid tab change would otherwise write
+      // the previous tab's arrangement under the new tab's number by this path
+      // instead.
+      const shouldPersistLockedLayouts = hasInitializedLockEffectRef.current
+        && !wasLocked
+        && locked
+        && layoutTabRef.current === activeTab;
       if (shouldPersistLockedLayouts) {
         saveLayoutsToApi(updatedLayout, activeTab, gridCols);
       }
@@ -260,7 +206,16 @@ const WidgetContainer = ({
   }, [locked]);
 
   const handleLayoutChange = (newLayout) => {
-    if (locked) return;
+    // Not just `locked`: the grid also emits during a tab change, before the
+    // rebuild for the new tab has landed. Saving then writes the previous tab's
+    // arrangement under the new tab's number.
+    if (!shouldAcceptLayoutChange({
+      locked,
+      layoutTab: layoutTabRef.current,
+      activeTab,
+    })) {
+      return;
+    }
 
     const currentLayoutById = new Map(layout.map(item => [item.i, item]));
     const safeLayout = newLayout.map((item) => {
@@ -491,6 +446,35 @@ const WidgetContainer = ({
     }));
   }, []);
 
+  // The layout handed to the grid must describe exactly the children being
+  // rendered — one entry each, no more.
+  //
+  // `layout` state is rebuilt by an effect, so during a tab change it still
+  // describes the previous tab while the children are already the new tab's.
+  // Passing it raw gives the grid entries whose `i` matches no child, and
+  // children with no entry; it then synthesizes placements and, with
+  // preventCollision, shuffles non-static items around until they fit. That is
+  // the visible scramble, and it only appears unlocked because static items are
+  // pinned and excluded from collision movement.
+  //
+  // Deriving it per render closes the window: the grid never sees one tab's
+  // items alongside another tab's children.
+  // The layout handed to the grid must describe exactly the children being
+  // rendered. `layout` state is rebuilt by an effect, so during a tab change it
+  // still describes the previous tab — passing it raw gives the grid entries
+  // matching no child and children with no entry, and it shuffles non-static
+  // items around hunting for a fit. That is the visible scramble, and it only
+  // appears unlocked because static items are pinned.
+  //
+  // Built from `widgets`, whose savedLayout is already scoped to the active tab,
+  // so it is correct even mid-transition. Once the rebuild has landed for this
+  // tab, live state wins so a drag in progress is not thrown away.
+  const gridLayout = useMemo(() => {
+    const built = buildLayout(widgets, gridCols, locked);
+    if (layoutTabRef.current !== activeTab) return built;
+    return built.map((item) => layout.find((l) => l.i === item.i) || item);
+  }, [widgets, layout, gridCols, locked, activeTab]);
+
   const resizeButtonBaseStyle = {
     fontSize: '1.5rem',
     userSelect: 'none',
@@ -532,7 +516,7 @@ const WidgetContainer = ({
         <GridLayout
           className="layout"
           width={containerWidth}
-          layout={layout}
+          layout={gridLayout}
           gridConfig={{
             cols: gridCols,
             rowHeight: 100,

client/src/utils/gridPlacement.js

@@ -0,0 +1,69 @@
+import { layoutItemFromNormalized } from './gridLayout.js';
+
+// Build a complete grid layout for a set of widgets.
+//
+// One entry per widget, never overlapping. A widget with a saved layout keeps
+// it; one without is placed in the first free cell rather than at its default
+// position, because defaults are not unique — every plugin widget declares
+// (0,0) at 6x4, so honouring them naively stacks the whole plugin set in one
+// place.
+//
+// `widget.savedLayout` is already scoped to the active tab by the caller, which
+// is what makes this safe to call during a tab change: it describes the tab
+// being rendered, not whatever the previous layout state happened to hold.
+export function buildLayout(widgets, cols, locked) {
+  const placed = [];
+
+  const collides = (x, y, w, h) => placed.some(
+    (p) => x < p.x + p.w && x + w > p.x && y < p.y + p.h && y + h > p.y
+  );
+
+  const findFreePosition = (w, h) => {
+    for (let row = 0; row < 200; row++) {
+      for (let col = 0; col <= cols - w; col++) {
+        if (!collides(col, row, w, h)) return { x: col, y: row };
+      }
+    }
+    return { x: 0, y: 0 };
+  };
+
+  return widgets.map((widget) => {
+    const minW = widget.minWidth || 3;
+    const minH = widget.minHeight || 2;
+    const source = widget.savedLayout
+      ? {
+        x: widget.savedLayout.x ?? widget.defaultPosition.x,
+        y: widget.savedLayout.y ?? widget.defaultPosition.y,
+        w: widget.savedLayout.w || widget.defaultSize.width,
+        h: widget.savedLayout.h || widget.defaultSize.height,
+        minW,
+        minH,
+      }
+      : {
+        x: widget.defaultPosition.x,
+        y: widget.defaultPosition.y,
+        w: widget.defaultSize.width,
+        h: widget.defaultSize.height,
+        minW,
+        minH,
+      };
+
+    const scaled = layoutItemFromNormalized(source, cols);
+    const pos = widget.savedLayout
+      ? { x: scaled.x, y: scaled.y }
+      : findFreePosition(scaled.w, scaled.h);
+
+    const item = {
+      i: widget.id,
+      x: pos.x,
+      y: pos.y,
+      w: scaled.w,
+      h: scaled.h,
+      minW: scaled.minW,
+      minH: scaled.minH,
+      static: locked,
+    };
+    placed.push({ x: item.x, y: item.y, w: item.w, h: item.h });
+    return item;
+  });
+}

client/src/utils/gridPlacement.test.js

@@ -0,0 +1,63 @@
+import { describe, it, expect } from 'vitest';
+import { buildLayout } from './gridPlacement';
+
+const plugin = (id) => ({
+  id,
+  defaultPosition: { x: 0, y: 0 },
+  defaultSize: { width: 6, height: 4 },
+  minWidth: 2,
+  minHeight: 2,
+});
+
+const overlaps = (items) => {
+  for (let i = 0; i < items.length; i += 1) {
+    for (let j = i + 1; j < items.length; j += 1) {
+      const a = items[i]; const b = items[j];
+      if (a.x < b.x + b.w && a.x + a.w > b.x && a.y < b.y + b.h && a.y + a.h > b.y) {
+        return [a.i, b.i];
+      }
+    }
+  }
+  return null;
+};
+
+describe('buildLayout', () => {
+  // The regression this helper exists to prevent: every plugin widget declares
+  // defaultPosition (0,0) and defaultSize 6x4, so placing them at their defaults
+  // stacks the entire plugin set in one cell.
+  it('does not stack widgets that share identical defaults', () => {
+    const out = buildLayout([plugin('a'), plugin('b'), plugin('c')], 12, false);
+    expect(out).toHaveLength(3);
+    expect(overlaps(out)).toBeNull();
+  });
+
+  it('keeps a saved layout rather than re-placing it', () => {
+    const w = { ...plugin('a'), savedLayout: { x: 6, y: 9, w: 6, h: 4 } };
+    const [item] = buildLayout([w], 12, false);
+    expect({ x: item.x, y: item.y, w: item.w, h: item.h }).toEqual({ x: 6, y: 9, w: 6, h: 4 });
+  });
+
+  it('mixes saved and unsaved widgets without collisions', () => {
+    const saved = { ...plugin('saved'), savedLayout: { x: 0, y: 0, w: 12, h: 5 } };
+    const out = buildLayout([saved, plugin('new1'), plugin('new2')], 12, false);
+    expect(overlaps(out)).toBeNull();
+    // the unsaved ones must go below the full-width saved one, not on top of it
+    expect(out[1].y).toBeGreaterThanOrEqual(5);
+    expect(out[2].y).toBeGreaterThanOrEqual(5);
+  });
+
+  it('returns exactly one entry per widget, in order', () => {
+    const out = buildLayout([plugin('a'), plugin('b')], 12, false);
+    expect(out.map((i) => i.i)).toEqual(['a', 'b']);
+  });
+
+  it('marks items static to match the lock state', () => {
+    expect(buildLayout([plugin('a')], 12, true)[0].static).toBe(true);
+    expect(buildLayout([plugin('a')], 12, false)[0].static).toBe(false);
+  });
+
+  it('never places an item past the right edge', () => {
+    const out = buildLayout([plugin('a'), plugin('b'), plugin('c')], 12, false);
+    out.forEach((i) => expect(i.x + i.w).toBeLessThanOrEqual(12));
+  });
+});

client/src/utils/layoutSync.js

@@ -0,0 +1,23 @@
+// react-grid-layout fires `onLayoutChange` whenever its children change, not
+// only when a person drags or resizes something. Switching tabs swaps the whole
+// child set, so the grid emits a layout for the *new* tab while the component's
+// layout state still describes the *old* one — and saving that mixture writes
+// one tab's arrangement over another's.
+//
+// The state and the tab it was built for must agree before a layout change can
+// be trusted. `layoutTab` is the tab the current layout state was rebuilt for;
+// it is null until the first rebuild completes.
+
+export function shouldAcceptLayoutChange({ locked, layoutTab, activeTab }) {
+  // A locked dashboard has no drag or resize affordances, so any layout event is
+  // the grid reacting to something other than a person.
+  if (locked) return false;
+
+  // No layout has been built yet — nothing to compare an incoming change against.
+  if (layoutTab === null || layoutTab === undefined) return false;
+
+  // The layout state belongs to a different tab than the one now active: a tab
+  // change is in flight and the rebuild has not landed. Anything the grid emits
+  // here describes neither tab correctly.
+  return layoutTab === activeTab;
+}

client/src/utils/layoutSync.test.js

@@ -0,0 +1,34 @@
+import { describe, it, expect } from 'vitest';
+import { shouldAcceptLayoutChange } from './layoutSync';
+
+describe('shouldAcceptLayoutChange', () => {
+  it('accepts a change when the layout state belongs to the active tab', () => {
+    expect(shouldAcceptLayoutChange({ locked: false, layoutTab: 1, activeTab: 1 })).toBe(true);
+  });
+
+  it('rejects everything while locked, where there is no drag affordance', () => {
+    expect(shouldAcceptLayoutChange({ locked: true, layoutTab: 1, activeTab: 1 })).toBe(false);
+  });
+
+  // The bug: unlocked, mid tab change, the grid emits a layout for the new tab
+  // while state still holds the old one. Accepting it writes one tab over another.
+  it('rejects a change while a tab switch is in flight', () => {
+    expect(shouldAcceptLayoutChange({ locked: false, layoutTab: 2, activeTab: 1 })).toBe(false);
+    expect(shouldAcceptLayoutChange({ locked: false, layoutTab: 1, activeTab: 2 })).toBe(false);
+  });
+
+  it('rejects before the first rebuild has established a tab', () => {
+    expect(shouldAcceptLayoutChange({ locked: false, layoutTab: null, activeTab: 1 })).toBe(false);
+    expect(shouldAcceptLayoutChange({ locked: false, layoutTab: undefined, activeTab: 1 })).toBe(false);
+  });
+
+  // Tab 0 is falsy; a truthiness check instead of an explicit null check would
+  // reject a legitimate change on it.
+  it('accepts tab 0, which is falsy but valid', () => {
+    expect(shouldAcceptLayoutChange({ locked: false, layoutTab: 0, activeTab: 0 })).toBe(true);
+  });
+
+  it('does not coerce a numeric tab to its string form', () => {
+    expect(shouldAcceptLayoutChange({ locked: false, layoutTab: 1, activeTab: '1' })).toBe(false);
+  });
+});