From 82d649bff9552cb0fa2c29420cf2616b58cbea64 Mon Sep 17 00:00:00 2001 From: mrramam Date: Mon, 14 Sep 2026 21:24:48 -0700 Subject: [PATCH] fix(client): stop tab changes corrupting and scrambling widget layouts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Switching tabs with the dashboard unlocked overwrote the destination tab's saved layout, and visibly shuffled the widgets while doing it. Both came from the same place: the grid being handed a layout that did not describe the children it was rendering. `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 straight to the grid gives entries whose `i` matches no child, and children with no entry at all. With preventCollision set, the grid synthesises placements for the orphans and moves non-static items around until nothing collides — the visible scramble. It then reports that arrangement through onLayoutChange, and it was persisted under the new tab's number. Widgets present on both tabs inherited the previous tab's coordinates; widgets only on the destination tab fell through to `item.minW ?? 2` and collapsed to 2x2. On a two-tab dashboard of six widgets and two, one unlocked switch destroyed the six-widget tab and left three overlapping pairs. It only happened unlocked because static items are pinned and excluded from collision movement, which is why a wall display never showed it. Two changes: `buildLayout` derives the layout during render, one entry per child, from `widget.savedLayout` — which the caller has already scoped to the active tab, so it is correct even mid-transition. A widget without a saved layout takes the first free cell rather than its default position: defaults are not unique, every plugin widget declares (0,0) at 6x4, and honouring them naively stacks the whole plugin set in one place. The rebuild effect now uses the same helper instead of its own copy of the placement loop. `shouldAcceptLayoutChange` refuses to persist a layout whose tab does not match the active one. The derived layout should prevent a bad arrangement from being reported at all, but writing one tab's geometry under another's number is the damaging outcome here, so it is worth refusing outright. The lock transition takes the same check, since it persists on its own trigger. Tab 0 is falsy and valid, so the "no layout yet" case is an explicit null check rather than a truthiness test. Verified on a live deployment: the corruption was deterministic beforehand - byte-identical damage from two different routes to the same tab - and after the change a tab switch writes nothing, a drag still persists, and no overlaps are created. --- client/src/components/WidgetContainer.jsx | 132 ++++++++++------------ client/src/utils/gridPlacement.js | 69 +++++++++++ client/src/utils/gridPlacement.test.js | 63 +++++++++++ client/src/utils/layoutSync.js | 23 ++++ client/src/utils/layoutSync.test.js | 34 ++++++ 5 files changed, 247 insertions(+), 74 deletions(-) create mode 100644 client/src/utils/gridPlacement.js create mode 100644 client/src/utils/gridPlacement.test.js create mode 100644 client/src/utils/layoutSync.js create mode 100644 client/src/utils/layoutSync.test.js diff --git a/client/src/components/WidgetContainer.jsx b/client/src/components/WidgetContainer.jsx index 5625ac9..62a418d 100644 --- a/client/src/components/WidgetContainer.jsx +++ b/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 = ({ 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; + }); +} diff --git a/client/src/utils/gridPlacement.test.js b/client/src/utils/gridPlacement.test.js new file mode 100644 index 0000000..e7e1c25 --- /dev/null +++ b/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)); + }); +}); diff --git a/client/src/utils/layoutSync.js b/client/src/utils/layoutSync.js new file mode 100644 index 0000000..52e36f1 --- /dev/null +++ b/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; +} diff --git a/client/src/utils/layoutSync.test.js b/client/src/utils/layoutSync.test.js new file mode 100644 index 0000000..1e08e1d --- /dev/null +++ b/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); + }); +});