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); + }); +});