diff --git a/.changelog/next/changed-issue-4133.md b/.changelog/next/changed-issue-4133.md new file mode 100644 index 0000000000..a27beb56e1 --- /dev/null +++ b/.changelog/next/changed-issue-4133.md @@ -0,0 +1 @@ +- Dashboard grid layouts now store a single reading/packing sequence (`order`) instead of a row coordinate — drag, resize and mobile reorder all run in one coordinate space, and migration 269 converts saved layouts in place diff --git a/client/src/components/dashboard/CLAUDE.md b/client/src/components/dashboard/CLAUDE.md index 46758ceebc..4340149234 100644 --- a/client/src/components/dashboard/CLAUDE.md +++ b/client/src/components/dashboard/CLAUDE.md @@ -2,13 +2,15 @@ Dashboard widgets are registered in `widgetRegistry.jsx` — each entry has `{ id, label, Component, width, defaultH?, gate? }`. The Dashboard page renders the active layout's widget list from this registry; named layouts persist in `data/dashboard-layouts.json` and are managed via `GET/PUT/DELETE /api/dashboard/layouts`. Built-in layouts (`default`, `focus`, `morning-review`, `ops`) are seeded on first read and cannot be deleted. -**Grid positions:** layouts also carry a `grid: [{ id, x, y, w, h, fixedH? }]` array — free-form positions on a 12-column grid (rows ~80px each). When `grid` is empty (legacy/unmigrated layouts) the renderer auto-flows widgets using `synthesizeGrid` based on each widget's `width` keyword and `defaultH`. The "Arrange" button on the Dashboard enters edit mode where every widget exposes a move (top-right) and resize (bottom-right) handle; drag is snap-to-grid with collision-resolve via `placeAndCompact` (pins the moved item, slots others into the smallest non-colliding y). Save persists to the active layout's `grid`. The grid renderer collapses to a single-column stack below 640px container width, rendered in reading order (`y`, then `x`) — never grid-array order, which `placeAndCompact` deliberately scrambles by hoisting the moved item to the front. +**Grid positions:** layouts also carry a `grid: [{ id, x, w, order, h?, fixedH? }]` array — column placement on a 12-column grid plus a reading/packing sequence. When `grid` is empty (legacy/unmigrated layouts) the renderer auto-flows widgets using `synthesizeGrid` based on each widget's `width` keyword and `defaultH`. The "Arrange" button on the Dashboard enters edit mode where every widget exposes a move (top-right) and resize (bottom-right) handle; a move snaps horizontally to columns and re-inserts the cell into the sequence at the rank its dropped pixel position implies (`rankAtPixel` → `insertAtOrder`), renumbering `order` densely. Save persists to the active layout's `grid`. The grid renderer collapses to a single-column stack below 640px container width, rendered in reading order (`order`, then `x` as a tiebreak) — never grid-array order, which nothing keeps sorted. -**Height is measured, not declared.** `h` is NOT the rendered height. A cell measures its widget's natural height with a `ResizeObserver` and `packVertically` positions everything in pixels — each cell floats up to just below the nearest already-placed cell that shares a column. That's what stops an 80px card from reserving a 400px slot and leaving a band of dead whitespace across the row. `h` survives as the pre-measurement fallback (first paint) and as what an older client — which knows nothing about `fixedH` — reads out of a saved layout, so every drag commit refreshes it from the measurement via `toPackSpace`. A cell whose height the user actually dragged carries `fixedH: true` and keeps `h` exactly, clipping content the way the whole grid used to; the ⌃⌄ handle (edit mode, pinned cells only) hands the height back to the content. Stored `y` no longer sets the pixel offset — it only decides reading order, which is also the packing order, which is why the visual result still matches the arrangement the user saved. `defaultH` in the registry is likewise only a first-paint ballpark. +**One vertical coordinate (#4133).** There is no stored row position. Horizontal placement is declared (`x`/`w`); vertical placement belongs entirely to `packVertically`, which lays cells out in pixels, and `order` says only which cell packs first. That is why a gesture never has to reconcile "where the cell says it is" with "where it is drawn" — only one of those exists. Persisted layouts predating this carried `{ x, y, w, h }`; `sanitizeGridItem`/`sequenceGrid` in `server/services/dashboardLayouts.js` still derive `order` from a legacy `y` on read (shape-probed, not version-flagged), and `scripts/migrations/269-dashboard-grid-order.js` rewrites the file on disk. **A new widget-seeding migration must append `{ id, x, w, order, h }`, not `{ x, y, w, h }`** — the older seed migrations (029/030/033/070/145/156/191) still compute `y` because they only ever run on pre-269 files. + +**Height is measured, not declared.** `h` is NOT the rendered height. A cell measures its widget's natural height with a `ResizeObserver` and `packVertically` positions everything in pixels — each cell floats up to just below the nearest already-placed cell that shares a column. That's what stops an 80px card from reserving a 400px slot and leaving a band of dead whitespace across the row. `h` survives as the pre-measurement fallback (first paint) and as what an older client — which knows nothing about `fixedH` — reads out of a saved layout, so every drag commit refreshes it from the measurement via `withMeasuredHeights`. A cell whose height the user actually dragged carries `fixedH: true` and keeps `h` exactly, clipping content the way the whole grid used to; the ⌃⌄ handle (edit mode, pinned cells only) hands the height back to the content. `defaultH` in the registry is likewise only a first-paint ballpark. **Mobile editing.** Free-form move/resize is desktop-only (a phone has no room for positional editing), but *reordering* works on mobile: edit mode swaps the two grid handles for a single reorder handle. The 1-D sort goes through **`@dnd-kit/sortable`** — the same `PointerSensor` + `KeyboardSensor` + `verticalListSortingStrategy` pairing as `cos/tabs/TasksTab.jsx` and `universeBuilder/InfluenceChipsInput.jsx` — which is what supplies touch, keyboard (space to lift, arrows to move), multi-pointer safety, screen-reader announcements and edge auto-scroll. **Do not hand-roll a pointer gesture for this**; the free-form 2-D drag stays hand-rolled only because arbitrary grid placement is not what a sortable list does. Use `dndTransformToCss` from `client/src/lib/dndTransform.js` in the style object, never `@dnd-kit/utilities`. -On drop, `onDragEnd` re-flows the whole grid through `reflowToOrder` so its reading order matches the new stack order, preserving each widget's `w`/`h`. That means a mobile reorder re-packs the desktop layout into row-flow — the only coherent way to express a one-dimensional edit as 2D coordinates, and the edit-mode hint says so. `synthesizeGrid` is built on the same helper. The handle keeps `touch-action: none`; without it the browser claims the pointer stream for scrolling and the gesture never starts. +On drop, `onDragEnd` re-flows the whole grid through `reflowToOrder` so its reading order matches the new stack order, preserving each widget's `w`/`h`. That means a mobile reorder re-packs the desktop layout into row-flow — the only coherent way to express a one-dimensional edit as column placement, and the edit-mode hint says so. `synthesizeGrid` is built on the same helper. The handle keeps `touch-action: none`; without it the browser claims the pointer stream for scrolling and the gesture never starts. **One source of truth for the mobile/desktop seam.** `DashboardGrid` measures its own *container* (`useContainerWidth`), which page padding makes narrower than the viewport — so a Tailwind `sm:` breakpoint disagrees with it in a ~30px band. Anything outside the grid that needs to know which affordance is live (the Dashboard's edit-mode hint) reads it from the `onLayoutModeChange` callback, never from a CSS breakpoint. diff --git a/client/src/components/dashboard/DashboardGrid.jsx b/client/src/components/dashboard/DashboardGrid.jsx index 122d624a6e..2e18e74e30 100644 --- a/client/src/components/dashboard/DashboardGrid.jsx +++ b/client/src/components/dashboard/DashboardGrid.jsx @@ -19,29 +19,33 @@ import { GRID_COLS, GRID_DEFAULT_H, WIDTH_TO_COLS, WIDGETS_BY_ID } from './widge import useContainerWidth from '../../hooks/useContainerWidth'; import { dndTransformToCss } from '../../lib/dndTransform'; -// Free-form 12-column grid with snap-to-grid drag, resize, and content-sized -// cells that float up into whatever whitespace is above them. +// Free-form 12-column grid with snap-to-column drag, resize, and +// content-sized cells that float up into whatever whitespace is above them. // -// Items: [{ id, x, y, w, h, fixedH? }] where x/y/w/h are integer grid units -// - x: 0..11 (column origin) -// - y: 0..n (row origin; ordering/authoring coordinate — see below) -// - w: 1..12 (column span) -// - h: 1..n (row span, each row ROW_HEIGHT_PX tall) +// Items: [{ id, x, w, order, h?, fixedH? }] +// - x: 0..11 (column origin, integer grid units) +// - w: 1..12 (column span, integer grid units) +// - order: 0..n (reading/packing order — the ONLY vertical coordinate) +// - h: 1..n (row span, each row ROW_HEIGHT_PX tall — fallback only) // - fixedH: the user dragged a height onto this cell; honor `h` exactly // +// ONE VERTICAL COORDINATE. Horizontal placement is declared (x/w); vertical +// placement is entirely owned by `packVertically`, which lays cells out in +// PIXELS. `order` says which cell packs first, and nothing else — there is no +// stored row position, no rectangle-collision pass, and no y-compaction. A +// gesture therefore never has to reconcile "where the cell says it is" with +// "where the cell is drawn," because only one of those exists. +// // HEIGHT IS MEASURED, NOT DECLARED. A widget's card is only as tall as its // content, so `h` is not the rendered height — it's the pre-measurement -// fallback (first paint, and what older clients read out of a saved layout). -// Cells the user has explicitly resized carry `fixedH: true` and keep their -// declared height, clipping content the way the whole grid used to. +// fallback (first paint, and what an older client reads out of a saved +// layout). Cells the user has explicitly resized carry `fixedH: true` and keep +// their declared height, clipping content the way the whole grid used to. // -// Columns are laid out from x/w in grid units; vertical position is packed in -// PIXELS by `packVertically` — each cell drops as high as it can go without -// landing on an already-placed cell that shares a column. That's what -// reclaims dead whitespace: an 80px-tall card in a 5-row slot no longer -// reserves 400px, and the cards below it float up through the gap. Stored `y` -// still decides reading order (and therefore packing order), so the visual -// result always matches the order the user arranged. +// The pack is what reclaims dead whitespace: each cell drops as high as it can +// go without landing on an already-placed cell that shares a column, so an +// 80px-tall card in a 5-row slot no longer reserves 400px and the cards below +// it float up through the gap. // // In edit mode each item exposes a top-right move handle and a bottom-right // resize handle — plus, on a cell that's already pinned, a bottom-left @@ -61,14 +65,11 @@ import { dndTransformToCss } from '../../lib/dndTransform'; // but the 1-D case is exactly dnd-kit's job, and going through it is what // buys touch, keyboard, multi-pointer and edge auto-scroll for free. // -// Collision policy after drag/resize (`placeAndCompact`): pin the moved item -// at its dropped position, then slot every other item into the smallest y that -// doesn't collide with anything already placed (top-left items processed -// first). Tetris-style compaction — same feel as react-grid-layout / -// gridstack. This runs in GRID UNITS and settles what gets persisted; the -// pixel pack above is what actually gets drawn. The two agree because a -// commit first re-expresses the layout in pack space (`toPackSpace`), so the -// y/h it compacts are the ones the pack produced. +// Drop policy after a move (`insertAtOrder`): the dragged cell is re-inserted +// into the reading sequence at the rank its dropped pixel position implies, +// and every cell is renumbered 0..n-1 from there. The live preview runs the +// same function on the same inputs, so what the drop lands on is exactly what +// was previewed — no compaction pass, no round-trip between coordinate spaces. const ROW_HEIGHT_PX = 80; const GAP_PX = 16; @@ -87,17 +88,36 @@ const EDIT_MIN_HEIGHT_PX = 48; // live gets it from `onLayoutModeChange`, because this threshold is measured // against the CONTAINER and re-deriving it outside would read the viewport. const MOBILE_BREAKPOINT_PX = 640; +// Vertical slack, in pixels, within which two cells count as "the same row" +// when a move drag is deciding where the dragged cell lands in the sequence. +// Half a row: past that the cursor has clearly committed to above/below, and +// inside it the column (x) is what orders them — which is what makes dragging +// a card sideways past its row neighbour reorder the two. +// +// Must stay under EDIT_MIN_HEIGHT_PX + GAP_PX (the closest two stacked cells +// can ever be in edit mode, which is the only mode a drag runs in). Above +// that, two cells in the SAME column could read as "same row," and a cell +// whose column ties would then re-rank itself on a zero-distance click. +const SAME_ROW_PX = ROW_HEIGHT_PX / 2; function getColWidth(containerWidth) { return (containerWidth - GAP_PX * (GRID_COLS - 1)) / GRID_COLS; } -// Reading order: top-to-bottom, then left-to-right. This is the order the -// single-column mobile stack renders in, and the order `reflowToOrder` -// consumes. `placeAndCompact` returns the moved item first regardless of -// where it landed, so grid array order can't be trusted for this. +// Reading order: `order`, then column as a tiebreak. This is the order the +// single-column mobile stack renders in, the order `packVertically` consumes, +// and the order `reflowToOrder` renumbers. The x tiebreak only matters while a +// layout is momentarily sparse/duplicated (a widget appended by +// `reconcileGrid` before the next resequence) — a settled grid is dense. function byReadingOrder(a, b) { - return a.y - b.y || a.x - b.x; + return (a.order ?? 0) - (b.order ?? 0) || a.x - b.x; +} + +// Renumber `order` densely from the current reading order. Every function that +// hands a grid back to the caller ends here, so the persisted sequence is +// always 0..n-1 with no gaps and no duplicates. +function resequence(items) { + return [...items].sort(byReadingOrder).map((it, i) => ({ ...it, order: i })); } // Grid rows ↔ pixels. A span of `h` rows covers h row boxes plus the gaps @@ -113,7 +133,7 @@ export function pxToRows(px) { // Rendered height of one cell. Auto-height cells (the default) are exactly as // tall as they measured; `fixedH` cells keep the height the user dragged onto // them. `h` is the fallback until the first measurement lands — which is also -// why every commit refreshes it (see `toPackSpace`). +// why every commit refreshes it (see `withMeasuredHeights`). // // Keyed on PRESENCE, not truthiness: a widget that renders nothing (several // self-hide on empty data) measures a legitimate 0, and collapsing that into @@ -128,9 +148,6 @@ export function itemHeightPx(item, heights = {}) { function shareColumns(a, b) { return !(a.x + a.w <= b.x || b.x + b.w <= a.x); } -function shareRows(a, b) { - return !(a.y + a.h <= b.y || b.y + b.h <= a.y); -} // Masonry float. Walk cells in reading order and drop each one as high as it // will go without landing on an already-placed cell that shares a column. @@ -167,28 +184,66 @@ function columnRect(item, colWidth) { }; } -// Re-express the layout in the pack's own coordinate space: `y` becomes the -// row the cell actually renders at and `h` the rows it actually occupies. -// Once heights are measured, stored y/h are only an ordering hint and a -// fallback — a drag that reasoned in them would move the cursor and the card -// at different speeds, and would compare the dragged cell's position against -// coordinates nothing is drawn at. Every commit goes through here, which is -// also what keeps persisted y/h honest for the next cold load (and for a -// client too old to know about `fixedH`). -function toPackSpace(items, rects) { +// Refresh each item's `h` from the height the pack actually gave it. `h` is +// only a fallback now (first paint, and what a client too old to know about +// `fixedH` renders), so it would rot without this — every commit goes through +// here to keep the next cold load opening at roughly the right size. It is +// also what makes a resize gesture start where the card is DRAWN: grabbing the +// handle on an auto-height cell would otherwise snap it to a stale row count +// before the pointer had moved. +function withMeasuredHeights(items, rects) { return items.map((it) => { const rect = rects.get(it.id); if (!rect) return { ...it }; - return { ...it, y: Math.round(rect.top / ROW_STEP_PX), h: pxToRows(rect.height) }; + return { ...it, h: pxToRows(rect.height) }; }); } -function overlaps(a, b) { - return shareColumns(a, b) && shareRows(a, b); +// Where a cell dragged to pixel `top` in column `x` lands in the reading +// sequence: the number of other cells that sort before it. Cells clearly above +// or below (more than half a row away) are ordered by pixel position; cells on +// the same row are ordered by column. +function rankAtPixel(items, rects, movedId, top, x) { + let rank = 0; + for (const it of items) { + if (it.id === movedId) continue; + const otherTop = rects.get(it.id)?.top ?? 0; + const before = otherTop < top - SAME_ROW_PX + || (otherTop <= top + SAME_ROW_PX && it.x < x); + if (before) rank += 1; + } + return rank; +} + +// Re-insert `movedId` into the reading sequence at `rank`, then renumber. +// Both the live drag preview and the drop commit call this with the same +// inputs, so the preview is the outcome rather than an approximation of it. +function insertAtOrder(items, movedId, rank) { + const moved = items.find((it) => it.id === movedId); + if (!moved) return resequence(items); + const rest = [...items].filter((it) => it.id !== movedId).sort(byReadingOrder); + const next = [...rest.slice(0, rank), moved, ...rest.slice(rank)]; + return next.map((it, i) => ({ ...it, order: i })); +} + +// Has the gesture actually moved the cell? Compares everything a drag can +// change — the column pair, the fallback height, and the sequence position. +function samePlacement(a, b) { + return a.x === b.x && a.w === b.w && a.h === b.h && a.order === b.order; } -function sameRect(a, b) { - return a.x === b.x && a.y === b.y && a.w === b.w && a.h === b.h; +// Fold the in-flight ghost back into the baseline. A move re-inserts at the +// ghost's rank and renumbers; a resize touches only that cell's w/h, so the +// sequence is left alone. `pin` marks the cell `fixedH` — always true while a +// resize is in flight (the drag, not the content, defines the height for the +// duration of the gesture) but only true on drop if the height really changed. +function applyGhost(baseline, kind, ghost, pin) { + const swapped = baseline.map((it) => (it.id === ghost.id + ? { ...it, ...ghost, ...(pin ? { fixedH: true } : {}) } + : it)); + return kind === 'resize' + ? resequence(swapped) + : insertAtOrder(swapped, ghost.id, ghost.order); } // Everything that differs between the three handles, on one row each. `kind` @@ -238,58 +293,37 @@ function DragHandle({ kind, item, onPointerDown, onClick, handleProps }) { ); } -// Pin the moved item at its dropped position, then slide every other item -// upward to the smallest y that doesn't collide with anything already -// placed. Combines collision-resolve and compact in one pass: the moved -// item goes first (so it acts as an obstacle for everyone else) and the -// rest are processed in current (y, x) order so top-left items keep -// precedence. Returns a new array — never mutates input. -function placeAndCompact(items, movedId) { - const moved = items.find((i) => i.id === movedId); - if (!moved) return items.map((it) => ({ ...it })); - const rest = items.filter((i) => i.id !== movedId).sort(byReadingOrder); - const placed = [{ ...moved }]; - for (const item of rest) { - let y = 0; - while (placed.some((p) => overlaps({ ...item, y }, p))) y += 1; - placed.push({ ...item, y }); - } - return placed; -} - -// Auto-place a new widget at the bottom of the grid, left-aligned. Used when -// LayoutEditor adds a widget to a layout without specifying coordinates. +// Auto-place a new widget at the end of the reading sequence, left-aligned. +// Used when LayoutEditor adds a widget to a layout without specifying a +// position. The pack decides where "the end" actually renders. export function placeNewWidget(items, widgetId) { const meta = WIDGETS_BY_ID[widgetId]; const w = WIDTH_TO_COLS[meta?.width] ?? 4; const h = meta?.defaultH ?? GRID_DEFAULT_H; - const bottom = items.reduce((max, it) => Math.max(max, it.y + it.h), 0); - return [...items, { id: widgetId, x: 0, y: bottom, w, h }]; + const last = items.reduce((max, it) => Math.max(max, (it.order ?? 0) + 1), 0); + return [...items, { id: widgetId, x: 0, w, h, order: last }]; } -// Row-flow items into the 12-column grid following `orderedIds`, preserving +// Row-flow items into the 12 columns following `orderedIds`, preserving // each item's w/h and dropping anything not in the order. This is how a mobile -// reorder becomes a grid: the single-column stack has no x/y to drop onto, so -// the new stack order is re-flowed into fresh coordinates. Items that already -// sit in flow order (the common case) come back with the same coordinates. +// reorder becomes a grid: the single-column stack has no columns to drop onto, +// so the new stack order is re-flowed into fresh x positions and a fresh +// sequence. Items that already sit in flow order (the common case) come back +// unchanged. export function reflowToOrder(items, orderedIds = items.map((it) => it.id)) { const byId = new Map(items.map((it) => [it.id, it])); const flowed = []; let cursorX = 0; - let cursorY = 0; - let rowMaxH = 0; for (const id of orderedIds) { const item = byId.get(id); if (!item) continue; const w = Math.min(item.w, GRID_COLS); - if (cursorX + w > GRID_COLS) { - cursorX = 0; - cursorY += rowMaxH; - rowMaxH = 0; - } - flowed.push({ ...item, x: cursorX, y: cursorY, w }); + // Wrap to a fresh row when the widget no longer fits beside its + // predecessor. There is no row COORDINATE to advance — the pack derives + // the row from the sequence — only the column cursor to reset. + if (cursorX + w > GRID_COLS) cursorX = 0; + flowed.push({ ...item, x: cursorX, w, order: flowed.length }); cursorX += w; - rowMaxH = Math.max(rowMaxH, item.h); } return flowed; } @@ -331,7 +365,11 @@ export function reconcileGrid(grid, visibleIds, { reorder = false } = {}) { if (present.has(id)) continue; kept = placeNewWidget(kept, id); } - return reorder ? reflowToOrder(kept, visibleIds) : kept; + // Either way the sequence comes back dense — `reflowToOrder` renumbers from + // the list, and `resequence` closes the gaps that dropping entries leaves + // (appending can also duplicate the last index) — so callers never have to + // care. + return reorder ? reflowToOrder(kept, visibleIds) : resequence(kept); } // Reading order of a grid — what the mobile stack shows, and what a layout's @@ -451,12 +489,13 @@ export default function DashboardGrid({ items, editable, onChange, onLayoutModeC // pointermove (would spam re-renders of every widget). React only learns // about the new ghost when we call setDrag, throttled by RAF. // - // `drag` is `{ kind, baseline, ghost }`. `baseline` is the layout as it - // stood when the gesture started, in pack space (see `toPackSpace`): - // everything the drag reasons about — the ghost, the live preview, the - // committed grid — is relative to it, not to the stored coordinates, which - // no longer describe where anything is drawn. `ghost` stays a plain grid - // item so the commit can spread it straight onto one. + // `drag` is `{ kind, baseline, baseRects, ghost }`. `baseline` is the layout + // as it stood when the gesture started with its `h` refreshed from the + // measurement (see `withMeasuredHeights`), and `baseRects` the pixel rects + // the pack had produced for it. Everything the drag reasons about — the + // ghost's rank, the live preview, the committed grid — is relative to that + // frozen snapshot, never to the live rects, which the preview itself moves. + // `ghost` stays a plain grid item so the commit can spread it straight on. const dragRef = useRef(null); const [drag, setDrag] = useState(null); // Measured natural heights, px, keyed by widget id. @@ -484,14 +523,11 @@ export default function DashboardGrid({ items, editable, onChange, onLayoutModeC // cursor swapped for its snapped ghost, so every OTHER cell reflows live to // where it will land instead of waiting for the drop. A resize ghost packs // as a pinned cell — the drag is defining a height, so the preview must - // show that height and not the content's. + // show that height and not the content's. `applyGhost` is shared with the + // drop commit, so the preview IS the outcome rather than a lookalike of it. const previewItems = useMemo(() => { if (!drag) return ordered; - return drag.baseline - .map((it) => (it.id === drag.ghost.id - ? { ...it, ...drag.ghost, ...(drag.kind === 'resize' ? { fixedH: true } : {}) } - : it)) - .sort(byReadingOrder); + return applyGhost(drag.baseline, drag.kind, drag.ghost, drag.kind === 'resize'); }, [ordered, drag]); const rects = useMemo( @@ -527,24 +563,38 @@ export default function DashboardGrid({ items, editable, onChange, onLayoutModeC // never let the pointer leave the gesture. e.preventDefault(); e.stopPropagation(); - // Snapshot in pack space so the ghost starts exactly where the card is - // drawn — grabbing the resize handle on an auto-height cell would - // otherwise snap it to a stale row count before the pointer has moved. - const baseline = toPackSpace(items, rectsRef.current); + // Freeze the pack's output for the gesture: the ghost has to start exactly + // where the card is DRAWN (grabbing the resize handle on an auto-height + // cell would otherwise snap it to a stale row count before the pointer + // moved), and the rank math has to compare against positions the preview + // isn't simultaneously shifting. + const baseRects = rectsRef.current; + const baseline = withMeasuredHeights(items, baseRects); const found = baseline.find((it) => it.id === item.id) ?? item; // A resize gesture's floor applies to where the drag STARTS too. Without // it, a cell measuring under MIN_H rows (a widget rendering little or // nothing) has its height "changed" by the clamp alone — so a purely // horizontal drag would silently pin it. - const startItem = kind === 'resize' ? { ...found, h: Math.max(MIN_H, found.h) } : found; + // + // A move's start is expressed as the rank the cell ALREADY reads at, not + // its stored `order`. The two can differ: the pack places in sequence, but + // a cell sharing no column with any predecessor lands at the top anyway, + // so a later `order` can be DRAWN above an earlier one. Comparing a + // pixel-derived rank against a stored one there would "reorder" a card on + // a plain click of the handle — a write with nothing behind it. + const startTop = baseRects.get(item.id)?.top ?? 0; + const startItem = kind === 'resize' + ? { ...found, h: Math.max(MIN_H, found.h) } + : { ...found, order: rankAtPixel(baseline, baseRects, item.id, startTop, found.x) }; dragRef.current = { id: item.id, kind, startPointer: { x: e.clientX, y: e.clientY }, + startTop, startItem, ghost: { ...startItem }, }; - setDrag({ kind, baseline, ghost: { ...startItem } }); + setDrag({ kind, baseline, baseRects, ghost: { ...startItem } }); }, [editable, isMobile, items]); useEffect(() => { @@ -553,7 +603,7 @@ export default function DashboardGrid({ items, editable, onChange, onLayoutModeC // Both are fixed for the life of the gesture, and this effect re-installs // exactly when one starts — so capturing them here can't go stale, and the // per-snap `setDrag` doesn't have to re-bind the window listeners. - const { kind, baseline } = drag; + const { kind, baseline, baseRects } = drag; let raf = 0; const onPointerMove = (e) => { @@ -567,8 +617,12 @@ export default function DashboardGrid({ items, editable, onChange, onLayoutModeC let next; if (kind === 'move') { const newX = Math.max(0, Math.min(GRID_COLS - start.w, Math.round(start.x + dx / colStep))); - const newY = Math.max(0, Math.round(start.y + dy / ROW_STEP_PX)); - next = { ...start, x: newX, y: newY }; + // Vertical is read straight off the pointer in pixels and turned into + // a rank — there is no row coordinate to snap to, and the cursor and + // the card therefore travel at the same speed. + const top = gesture.startTop + dy; + const order = rankAtPixel(baseline, baseRects, gesture.id, top, newX); + next = { ...start, x: newX, order }; } else { const newW = Math.max(MIN_W, Math.min(GRID_COLS - start.x, Math.round(start.w + dx / colStep))); const newH = Math.max(MIN_H, Math.round(start.h + dy / ROW_STEP_PX)); @@ -578,7 +632,7 @@ export default function DashboardGrid({ items, editable, onChange, onLayoutModeC // when the cursor crosses a snap boundary. Skip the React update // when we're still inside the same snap cell — saves ~60 widget // re-renders per drag and keeps the rAF callback a no-op. - if (sameRect(gesture.ghost, next)) return; + if (samePlacement(gesture.ghost, next)) return; gesture.ghost = next; if (!raf) { raf = requestAnimationFrame(() => { @@ -597,14 +651,11 @@ export default function DashboardGrid({ items, editable, onChange, onLayoutModeC if (!gesture || !commit) return; // Skip the write entirely when nothing actually changed — avoids a // 200 OK on every accidental click on the drag handle. - if (sameRect(gesture.startItem, gesture.ghost)) return; + if (samePlacement(gesture.startItem, gesture.ghost)) return; // A resize that actually changed the HEIGHT is the user declaring one: // pin it. A width-only resize (or a move) leaves the cell auto-sized. const pins = kind === 'resize' && gesture.ghost.h !== gesture.startItem.h; - const updated = baseline.map((it) => (it.id === gesture.id - ? { ...it, ...gesture.ghost, ...(pins ? { fixedH: true } : {}) } - : it)); - onChange(placeAndCompact(updated, gesture.id)); + onChange(applyGhost(baseline, kind, gesture.ghost, pins)); }; const onPointerUp = () => finish(true); @@ -646,14 +697,14 @@ export default function DashboardGrid({ items, editable, onChange, onLayoutModeC // unconstrained, the observer reports its natural height, and the pack // closes the gap on the next frame. const clearFixedHeight = useCallback((item) => { - const next = toPackSpace(items, rectsRef.current).map((it) => { + const next = withMeasuredHeights(items, rectsRef.current).map((it) => { if (it.id !== item.id) return it; // Drop the pin by omission rather than whitelisting the fields to keep, // so a field added to the grid item later doesn't get eaten here. const { fixedH: _pinned, ...rest } = it; return rest; }); - onChange(placeAndCompact(next, item.id)); + onChange(resequence(next)); }, [items, onChange]); const sortable = editable && isMobile; diff --git a/client/src/components/dashboard/DashboardGrid.test.jsx b/client/src/components/dashboard/DashboardGrid.test.jsx index c19f06344f..0af98cb3e7 100644 --- a/client/src/components/dashboard/DashboardGrid.test.jsx +++ b/client/src/components/dashboard/DashboardGrid.test.jsx @@ -77,9 +77,9 @@ afterEach(() => { }); const THREE = [ - { id: 'a', x: 0, y: 0, w: 12, h: 2 }, - { id: 'b', x: 0, y: 2, w: 12, h: 2 }, - { id: 'c', x: 0, y: 4, w: 12, h: 2 }, + { id: 'a', x: 0, w: 12, order: 0, h: 2 }, + { id: 'b', x: 0, w: 12, order: 1, h: 2 }, + { id: 'c', x: 0, w: 12, order: 2, h: 2 }, ]; function renderGrid(items = THREE) { @@ -98,23 +98,23 @@ function renderGrid(items = THREE) { const cellFor = (id) => document.querySelector(`[data-widget-id="${id}"]`); describe('reflowToOrder', () => { - it('renumbers coordinates to match the requested order, keeping w/h', () => { + it('renumbers the sequence to match the requested order, keeping w/h', () => { const flowed = reflowToOrder(THREE, ['c', 'a', 'b']); expect(flowed.map((it) => it.id)).toEqual(['c', 'a', 'b']); - expect(flowed.map((it) => it.y)).toEqual([0, 2, 4]); + expect(flowed.map((it) => it.order)).toEqual([0, 1, 2]); expect(flowed.every((it) => it.w === 12 && it.h === 2)).toBe(true); }); it('packs items that fit side by side into the same row', () => { const items = [ - { id: 'a', x: 0, y: 0, w: 6, h: 3 }, - { id: 'b', x: 6, y: 0, w: 6, h: 2 }, - { id: 'c', x: 0, y: 3, w: 6, h: 2 }, + { id: 'a', x: 0, w: 6, order: 0, h: 3 }, + { id: 'b', x: 6, w: 6, order: 1, h: 2 }, + { id: 'c', x: 0, w: 6, order: 2, h: 2 }, ]; expect(reflowToOrder(items, ['b', 'a', 'c'])).toEqual([ - { id: 'b', x: 0, y: 0, w: 6, h: 2 }, - { id: 'a', x: 6, y: 0, w: 6, h: 3 }, - { id: 'c', x: 0, y: 3, w: 6, h: 2 }, + { id: 'b', x: 0, w: 6, order: 0, h: 2 }, + { id: 'a', x: 6, w: 6, order: 1, h: 3 }, + { id: 'c', x: 0, w: 6, order: 2, h: 2 }, ]); }); @@ -124,12 +124,12 @@ describe('reflowToOrder', () => { }); it('drops ids with no matching item and ignores items absent from the order', () => { - expect(reflowToOrder(THREE, ['b', 'ghost'])).toEqual([{ id: 'b', x: 0, y: 0, w: 12, h: 2 }]); + expect(reflowToOrder(THREE, ['b', 'ghost'])).toEqual([{ id: 'b', x: 0, w: 12, order: 0, h: 2 }]); }); it('still flows a widget whose stored width exceeds the grid', () => { - expect(reflowToOrder([{ id: 'a', x: 0, y: 0, w: 20, h: 2 }], ['a'])) - .toEqual([{ id: 'a', x: 0, y: 0, w: 12, h: 2 }]); + expect(reflowToOrder([{ id: 'a', x: 0, w: 20, order: 0, h: 2 }], ['a'])) + .toEqual([{ id: 'a', x: 0, w: 12, order: 0, h: 2 }]); }); }); @@ -145,7 +145,7 @@ describe('row ↔ pixel conversion', () => { }); describe('itemHeightPx', () => { - const item = { id: 'a', x: 0, y: 0, w: 6, h: 4 }; + const item = { id: 'a', x: 0, w: 6, h: 4 }; it('prefers the measured height so a short card stops reserving its whole slot', () => { expect(itemHeightPx(item, { a: 90 })).toBe(90); @@ -174,8 +174,8 @@ describe('packVertically', () => { // under b rather than sitting at b's declared bottom edge. const rects = packVertically( [ - { id: 'b', x: 0, y: 0, w: 12, h: 4 }, - { id: 'c', x: 0, y: 4, w: 12, h: 2 }, + { id: 'b', x: 0, w: 12, h: 4 }, + { id: 'c', x: 0, w: 12, h: 2 }, ], { b: 100, c: 100 } ); @@ -185,9 +185,9 @@ describe('packVertically', () => { it('only stacks cells that share a column', () => { const rects = packVertically([ - { id: 'left', x: 0, y: 0, w: 6, h: 2 }, - { id: 'right', x: 6, y: 0, w: 6, h: 2 }, - { id: 'under', x: 6, y: 2, w: 6, h: 2 }, + { id: 'left', x: 0, w: 6, h: 2 }, + { id: 'right', x: 6, w: 6, h: 2 }, + { id: 'under', x: 6, w: 6, h: 2 }, ], { left: 400, right: 100, under: 100 }); // `right` is beside `left`, not below it — a tall left column must not // push the whole right-hand stack down. @@ -197,8 +197,8 @@ describe('packVertically', () => { it('lets a widget that renders nothing occupy nothing, and closes over it', () => { const rects = packVertically([ - { id: 'empty', x: 0, y: 0, w: 12, h: 5 }, - { id: 'below', x: 0, y: 5, w: 12, h: 2 }, + { id: 'empty', x: 0, w: 12, h: 5 }, + { id: 'below', x: 0, w: 12, h: 2 }, ], { empty: 0, below: 100 }); expect(rects.get('empty').height).toBe(0); expect(rects.get('below').top).toBe(16); @@ -206,7 +206,7 @@ describe('packVertically', () => { it('floors cells to a grabbable height when edit mode asks for one', () => { const rects = packVertically( - [{ id: 'empty', x: 0, y: 0, w: 12, h: 5 }, { id: 'below', x: 0, y: 5, w: 12, h: 2 }], + [{ id: 'empty', x: 0, w: 12, h: 5 }, { id: 'below', x: 0, w: 12, h: 2 }], { empty: 0, below: 100 }, 48 ); @@ -216,24 +216,34 @@ describe('packVertically', () => { it('clears every cell it overlaps, not just the last one placed', () => { const rects = packVertically([ - { id: 'a', x: 0, y: 0, w: 4, h: 2 }, - { id: 'b', x: 4, y: 0, w: 4, h: 2 }, - { id: 'wide', x: 0, y: 2, w: 8, h: 2 }, + { id: 'a', x: 0, w: 4, h: 2 }, + { id: 'b', x: 4, w: 4, h: 2 }, + { id: 'wide', x: 0, w: 8, h: 2 }, ], { a: 300, b: 100, wide: 100 }); expect(rects.get('wide').top).toBe(316); }); }); describe('readingOrderIds', () => { - it('sorts top-to-bottom then left-to-right without mutating the input', () => { + it('sorts by the stored sequence without mutating the input', () => { const grid = [ - { id: 'c', x: 0, y: 4, w: 6, h: 2 }, - { id: 'b', x: 6, y: 0, w: 6, h: 2 }, - { id: 'a', x: 0, y: 0, w: 6, h: 2 }, + { id: 'c', x: 0, w: 6, order: 2, h: 2 }, + { id: 'b', x: 6, w: 6, order: 1, h: 2 }, + { id: 'a', x: 0, w: 6, order: 0, h: 2 }, ]; expect(readingOrderIds(grid)).toEqual(['a', 'b', 'c']); expect(grid[0].id).toBe('c'); }); + + // A grid momentarily sharing an `order` (two entries appended by + // reconcileGrid before the next resequence) still has to come back in a + // stable left-to-right order rather than whatever the sort happened to do. + it('breaks a tied sequence by column', () => { + expect(readingOrderIds([ + { id: 'right', x: 6, w: 6, order: 0, h: 2 }, + { id: 'left', x: 0, w: 6, order: 0, h: 2 }, + ])).toEqual(['left', 'right']); + }); }); describe('synthesizeGrid', () => { @@ -247,13 +257,21 @@ describe('reconcileGrid', () => { expect(reconcileGrid(THREE, ['a', 'c']).map((it) => it.id)).toEqual(['a', 'c']); }); + // Dropping the middle entry leaves a hole at order 1; a hand-back with gaps + // would drift further on every add/remove cycle. + it('hands back a dense sequence after dropping and appending', () => { + const next = reconcileGrid(THREE, ['a', 'c', 'apps']); + expect(next.map((it) => it.id)).toEqual(['a', 'c', 'apps']); + expect(next.map((it) => it.order)).toEqual([0, 1, 2]); + }); + // The arranged layout below deliberately disagrees with its widget list: `b` - // sits above `a` in the grid while the list still says `['a','b','c']`, which - // is exactly the pre-`saveGridEdit` state #4132 warns about. + // leads the sequence while the list still says `['a','b','c']`, which is + // exactly the pre-`saveGridEdit` state #4132 warns about. const ARRANGED = [ - { id: 'b', x: 0, y: 0, w: 6, h: 3 }, - { id: 'a', x: 6, y: 0, w: 6, h: 2 }, - { id: 'c', x: 0, y: 3, w: 12, h: 2 }, + { id: 'b', x: 0, w: 6, order: 0, h: 3 }, + { id: 'a', x: 6, w: 6, order: 1, h: 2 }, + { id: 'c', x: 0, w: 12, order: 2, h: 2 }, ]; it('leaves an arranged grid alone when the save is not a reorder', () => { @@ -299,16 +317,16 @@ describe('DashboardGrid mobile reorder', () => { expect(onChange).toHaveBeenCalledTimes(1); const next = onChange.mock.calls[0][0]; expect(next.map((it) => it.id)).toEqual(['b', 'c', 'a']); - expect(next.map((it) => it.y)).toEqual([0, 2, 4]); + expect(next.map((it) => it.order)).toEqual([0, 1, 2]); }); it('reads the drag against reading order, not grid-array order', () => { - // placeAndCompact hoists the moved item to the front of the array, so a - // saved grid routinely arrives out of visual order. + // A saved grid routinely arrives out of visual order — nothing keeps the + // persisted array sorted, only `order` says what the sequence is. const onChange = renderGrid([ - { id: 'c', x: 0, y: 4, w: 12, h: 2 }, - { id: 'a', x: 0, y: 0, w: 12, h: 2 }, - { id: 'b', x: 0, y: 2, w: 12, h: 2 }, + { id: 'c', x: 0, w: 12, order: 2, h: 2 }, + { id: 'a', x: 0, w: 12, order: 0, h: 2 }, + { id: 'b', x: 0, w: 12, order: 1, h: 2 }, ]); expect(screen.getAllByTestId(/^widget-/).map((el) => el.textContent)).toEqual(['a', 'b', 'c']); @@ -352,8 +370,8 @@ describe('DashboardGrid cell sizing', () => { it('measures the widget free of the cell height, and stretches it when pinned', () => { renderGrid([ - { id: 'a', x: 0, y: 0, w: 12, h: 3 }, - { id: 'b', x: 0, y: 3, w: 12, h: 3, fixedH: true }, + { id: 'a', x: 0, w: 12, order: 0, h: 3 }, + { id: 'b', x: 0, w: 12, order: 1, h: 3, fixedH: true }, ]); const measured = (id) => screen.getByTestId(`widget-${id}`).parentElement; expect(measured('a').className).toBe('flow-root'); @@ -361,14 +379,14 @@ describe('DashboardGrid cell sizing', () => { }); it('honors an explicit height on a cell the user pinned', () => { - renderGrid([{ id: 'a', x: 0, y: 0, w: 12, h: 3, fixedH: true }]); + renderGrid([{ id: 'a', x: 0, w: 12, order: 0, h: 3, fixedH: true }]); expect(cellFor('a').style.height).toBe(`${rowsToPx(3)}px`); }); it('offers the auto-fit handle only on pinned cells', () => { renderGrid([ - { id: 'a', x: 0, y: 0, w: 12, h: 3, fixedH: true }, - { id: 'b', x: 0, y: 3, w: 12, h: 3 }, + { id: 'a', x: 0, w: 12, order: 0, h: 3, fixedH: true }, + { id: 'b', x: 0, w: 12, order: 1, h: 3 }, ]); expect(screen.getByLabelText('Auto-fit height a')).toBeInTheDocument(); expect(screen.queryByLabelText('Auto-fit height b')).not.toBeInTheDocument(); @@ -376,12 +394,12 @@ describe('DashboardGrid cell sizing', () => { it('drops the pin — and keeps the last height as the new starting size — on auto-fit', () => { const onChange = renderGrid([ - { id: 'a', x: 0, y: 0, w: 12, h: 3, fixedH: true }, - { id: 'b', x: 0, y: 3, w: 12, h: 3 }, + { id: 'a', x: 0, w: 12, order: 0, h: 3, fixedH: true }, + { id: 'b', x: 0, w: 12, order: 1, h: 3 }, ]); fireEvent.click(screen.getByLabelText('Auto-fit height a')); const next = onChange.mock.calls[0][0]; - expect(next.find((it) => it.id === 'a')).toEqual({ id: 'a', x: 0, y: 0, w: 12, h: 3 }); + expect(next.find((it) => it.id === 'a')).toEqual({ id: 'a', x: 0, w: 12, order: 0, h: 3 }); expect(next.find((it) => it.id === 'b').fixedH).toBeUndefined(); }); }); @@ -407,7 +425,7 @@ describe('DashboardGrid resize pins the height', () => { }); it('leaves a width-only resize auto-sized', () => { - const onChange = renderGrid([{ id: 'a', x: 0, y: 0, w: 12, h: 2 }]); + const onChange = renderGrid([{ id: 'a', x: 0, w: 12, order: 0, h: 2 }]); // Narrow by four columns; the height never moves. dragHandle('Resize a', { dx: -4 * (1200 - 16 * 11) / 12 - 4 * 16 }); const moved = onChange.mock.calls[0][0].find((it) => it.id === 'a'); @@ -423,6 +441,73 @@ describe('DashboardGrid resize pins the height', () => { }); }); +// The move gesture is the whole point of collapsing the two coordinate +// systems: the drop position is read in pixels and turned straight into a +// sequence position, with no row coordinate and no compaction pass in between. +describe('DashboardGrid move re-sequences', () => { + beforeEach(() => { mockWidth = 1200; }); + + const dragHandle = (label, { dx = 0, dy = 0 }) => { + fireEvent.pointerDown(screen.getByLabelText(label), { clientX: 0, clientY: 0 }); + fireEvent.pointerMove(window, { clientX: dx, clientY: dy }); + fireEvent.pointerUp(window); + }; + + // Each stacked cell measures 100px and sits 116px below the last, so + // dragging the bottom card up past both of them lands it at rank 0. + it('moves a card to the front when dragged above everything else', () => { + const onChange = renderGrid(); + dragHandle('Move c', { dy: -400 }); + expect(onChange.mock.calls[0][0].map((it) => it.id)).toEqual(['c', 'a', 'b']); + expect(onChange.mock.calls[0][0].map((it) => it.order)).toEqual([0, 1, 2]); + }); + + it('slots a card between its neighbours rather than on top of one', () => { + const onChange = renderGrid(); + // Down past b's top edge (116) by more than the same-row slack — so a + // lands after b but well short of c (232). + dragHandle('Move a', { dy: 200 }); + expect(onChange.mock.calls[0][0].map((it) => it.id)).toEqual(['b', 'a', 'c']); + }); + + it('reorders side-by-side cards by the column they were dragged into', () => { + const onChange = renderGrid([ + { id: 'a', x: 0, w: 4, order: 0, h: 2 }, + { id: 'b', x: 4, w: 4, order: 1, h: 2 }, + { id: 'c', x: 8, w: 4, order: 2, h: 2 }, + ]); + // Eight columns right, no vertical movement: same row, so column decides. + const colStep = (1200 - 16 * 11) / 12 + 16; + dragHandle('Move a', { dx: colStep * 8 }); + const next = onChange.mock.calls[0][0]; + expect(next.map((it) => it.id)).toEqual(['b', 'a', 'c']); + expect(next.find((it) => it.id === 'a').x).toBe(8); + }); + + it('does not write when the card is dropped back where it started', () => { + const onChange = renderGrid(); + dragHandle('Move b', { dy: 4 }); + expect(onChange).not.toHaveBeenCalled(); + }); + + // The pack places in sequence, but a cell sharing no column with anything + // before it lands at the top regardless — so `c` (order 2) is DRAWN above + // `b` (order 1). A gesture that compared its pixel-derived rank against the + // stored order would commit a reorder for a card the user only clicked. + it('does not write when a card drawn above its stored order is only clicked', () => { + const onChange = renderGrid([ + { id: 'a', x: 0, w: 6, order: 0, h: 2 }, + { id: 'b', x: 0, w: 6, order: 1, h: 2 }, + { id: 'c', x: 6, w: 6, order: 2, h: 2 }, + ]); + expect(cellFor('c').style.top).toBe('0px'); + expect(cellFor('b').style.top).toBe(`${MEASURED_PX + 16}px`); + + dragHandle('Move c', { dy: 0 }); + expect(onChange).not.toHaveBeenCalled(); + }); +}); + describe('DashboardGrid scroll target', () => { beforeEach(() => { mockWidth = 1200; }); diff --git a/scripts/migrations/269-dashboard-grid-order.js b/scripts/migrations/269-dashboard-grid-order.js new file mode 100644 index 0000000000..a0208b9604 --- /dev/null +++ b/scripts/migrations/269-dashboard-grid-order.js @@ -0,0 +1,123 @@ +/** + * Convert persisted dashboard grids from the two-coordinate shape + * `{ id, x, y, w, h, fixedH? }` to the single-sequence shape + * `{ id, x, w, order, h?, fixedH? }` (issue #4133). + * + * Background: + * Dashboard cells have been content-measured and pixel-packed since the + * auto-height grid shipped, so the stored `y` stopped being a position and + * became nothing but a reading/packing order that the renderer had to keep + * agreeing with the pack on every gesture. This migration retires it: each + * layout's grid is sorted into its existing reading order (top-to-bottom, + * then left-to-right) and renumbered as a dense `order`, so a converted file + * opens in exactly the arrangement the user last saw. + * + * `h` is preserved untouched — it is still the first-paint fallback (and + * what a client too old to know about `fixedH` renders), which is why it + * survives the conversion at all. + * + * Idempotent: a grid whose every entry already carries `order` and no `y` is + * left alone. `server/services/dashboardLayouts.js` also derives `order` from + * `y` on read, so an install that has not run this migration (or a restored + * pre-#4133 backup) still gets usable geometry — this just makes the file on + * disk say what the renderer means. + */ + +import { + GRID_COLS, + GRID_ORDER_MAX, + GRID_LEGACY_Y_MAX, +} from '../../server/services/dashboardLayouts.js'; +import { readLayoutsDoc, writeLayoutsDoc } from './_lib.js'; + +const LABEL = 'migration 269'; + +const intOr = (v, fallback) => (Number.isFinite(v) ? Math.floor(v) : fallback); +// Bounds come from the service (the migration-030 convention) rather than +// being mirrored, so the two can't drift. The read path clamps BEFORE it +// ranks: two entries at y 300 and y 500 both land on the ceiling there and +// are separated by column instead, so ranking the raw values here would order +// them differently than the server already does. +const clamp = (v, max) => Math.max(0, Math.min(max, intOr(v, 0))); + +// Already-converted entries carry `order` and no `y`. Both halves matter: a +// hand-merged file can carry `order` alongside a stale `y`, and that still +// needs the rewrite so the dead coordinate stops shipping. +const isConverted = (grid) => + grid.every((g) => g && typeof g === 'object' && Number.isFinite(g.order) && g.y === undefined); + +// Resolve the sequence, probing the SHAPE exactly the way +// `sequenceGrid` in `server/services/dashboardLayouts.js` does — the two must +// agree, or a file converted here would read back in a different order than +// the same file read before conversion: +// - Fully legacy: reading order — top-to-bottom, then left-to-right — is +// the order the renderer already packed these cells in, so replaying it +// preserves the layout the user last saw. +// - Any entry carrying `order`: that's the sequence. Entries without one go +// last in file order (where a widget-seeding migration means to append). +// Sorting a half-converted grid by `y` instead would discard a real +// sequence and scramble the layout. +function toOrderedGrid(grid) { + const anyOrder = grid.some((g) => Number.isFinite(g.order)); + return grid + .map((g, idx) => ({ + g, + idx, + rank: anyOrder + ? (Number.isFinite(g.order) ? clamp(g.order, GRID_ORDER_MAX) : Number.MAX_SAFE_INTEGER) + : clamp(g.y, GRID_LEGACY_Y_MAX), + // Ties in legacy grids are side-by-side cells: column decides. Ties in + // the new shape are corrupt data: file order decides. + tie: anyOrder ? idx : clamp(g.x, GRID_COLS - 1), + })) + .sort((a, b) => a.rank - b.rank || a.tie - b.tie || a.idx - b.idx) + .map(({ g }, order) => { + const next = { id: g.id, x: intOr(g.x, 0), w: intOr(g.w, 1), order }; + if (Number.isFinite(g.h)) next.h = Math.floor(g.h); + if (g.fixedH === true) next.fixedH = true; + return next; + }); +} + +function applyToLayout(layout) { + if (!layout || typeof layout !== 'object') return false; + if (!Array.isArray(layout.grid) || layout.grid.length === 0) return false; + // A non-object entry is unusable to the renderer either way; drop it here + // rather than carrying `undefined` ids into the converted shape. Duplicates + // are dropped keeping the FIRST occurrence in FILE order, which is what the + // read path does — deduping after the sort would let conversion hand a + // different rectangle to a hand-edited duplicate than the server was + // already serving for it. + const seen = new Set(); + const grid = layout.grid.filter((g) => { + if (!g || typeof g !== 'object' || typeof g.id !== 'string') return false; + if (seen.has(g.id)) return false; + seen.add(g.id); + return true; + }); + if (grid.length === layout.grid.length && isConverted(grid)) return false; + layout.grid = toOrderedGrid(grid); + return true; +} + +export default { + async up({ rootDir }) { + const result = await readLayoutsDoc({ rootDir, label: LABEL }); + if (!result.ok) return { updated: 0, reason: result.reason }; + const { doc, path } = result; + + let touched = 0; + for (const layout of doc.layouts) { + if (applyToLayout(layout)) touched += 1; + } + + if (touched === 0) { + console.log(`📦 ${LABEL}: dashboard grids already use \`order\` — nothing to convert.`); + return { updated: 0, reason: 'already-applied' }; + } + + await writeLayoutsDoc(path, doc); + console.log(`📦 ${LABEL}: converted ${touched} dashboard layout grid(s) to the order-only shape.`); + return { updated: touched }; + }, +}; diff --git a/scripts/migrations/269-dashboard-grid-order.test.js b/scripts/migrations/269-dashboard-grid-order.test.js new file mode 100644 index 0000000000..d72ee9f854 --- /dev/null +++ b/scripts/migrations/269-dashboard-grid-order.test.js @@ -0,0 +1,191 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { mkdtempSync, rmSync, writeFileSync, readFileSync, mkdirSync, existsSync } from 'fs'; +import { tmpdir } from 'os'; +import { join } from 'path'; + +import migration from './269-dashboard-grid-order.js'; + +const writeJson = (path, value) => writeFileSync(path, JSON.stringify(value, null, 2) + '\n'); +const readJson = (path) => JSON.parse(readFileSync(path, 'utf-8')); + +describe('migration 269 — convert dashboard grids to the order-only shape', () => { + let rootDir; + let layoutsPath; + + beforeEach(() => { + rootDir = mkdtempSync(join(tmpdir(), 'migration-269-')); + mkdirSync(join(rootDir, 'data'), { recursive: true }); + layoutsPath = join(rootDir, 'data', 'dashboard-layouts.json'); + }); + + afterEach(() => { + rmSync(rootDir, { recursive: true, force: true }); + }); + + it('no-ops cleanly when dashboard-layouts.json is missing (fresh install)', async () => { + const result = await migration.up({ rootDir }); + expect(result.updated).toBe(0); + expect(result.reason).toBe('no-state'); + expect(existsSync(layoutsPath)).toBe(false); + }); + + it('replays reading order — top-to-bottom, then left-to-right — as a dense order', async () => { + writeJson(layoutsPath, { + activeLayoutId: 'default', + layouts: [{ + id: 'default', + name: 'Everything', + builtIn: true, + widgets: ['alpha', 'beta', 'gamma'], + // Deliberately stored out of visual order: a drag commit used to hoist + // the moved item to the front of the array. + grid: [ + { id: 'gamma', x: 0, y: 5, w: 12, h: 3 }, + { id: 'beta', x: 6, y: 0, w: 6, h: 2 }, + { id: 'alpha', x: 0, y: 0, w: 6, h: 4 }, + ], + }], + }); + + const result = await migration.up({ rootDir }); + expect(result.updated).toBe(1); + + const grid = readJson(layoutsPath).layouts[0].grid; + expect(grid).toEqual([ + { id: 'alpha', x: 0, w: 6, order: 0, h: 4 }, + { id: 'beta', x: 6, w: 6, order: 1, h: 2 }, + { id: 'gamma', x: 0, w: 12, order: 2, h: 3 }, + ]); + }); + + it('keeps the pinned-height flag and the fallback height', async () => { + writeJson(layoutsPath, { + activeLayoutId: 'default', + layouts: [{ + id: 'default', name: 'Everything', builtIn: true, widgets: ['alpha'], + grid: [{ id: 'alpha', x: 2, y: 3, w: 4, h: 7, fixedH: true }], + }], + }); + + await migration.up({ rootDir }); + expect(readJson(layoutsPath).layouts[0].grid[0]) + .toEqual({ id: 'alpha', x: 2, w: 4, order: 0, h: 7, fixedH: true }); + }); + + it('is idempotent — a converted grid is left alone on a second run', async () => { + writeJson(layoutsPath, { + activeLayoutId: 'default', + layouts: [{ + id: 'default', name: 'Everything', builtIn: true, widgets: ['alpha', 'beta'], + grid: [ + { id: 'alpha', x: 0, w: 6, order: 0, h: 4 }, + { id: 'beta', x: 6, w: 6, order: 1, h: 2 }, + ], + }], + }); + + const first = await migration.up({ rootDir }); + expect(first).toEqual({ updated: 0, reason: 'already-applied' }); + }); + + it('rewrites a half-converted grid so the dead `y` stops shipping', async () => { + writeJson(layoutsPath, { + activeLayoutId: 'default', + layouts: [{ + id: 'default', name: 'Everything', builtIn: true, widgets: ['alpha', 'beta'], + grid: [ + { id: 'alpha', x: 0, y: 0, w: 6, order: 0, h: 4 }, + { id: 'beta', x: 6, w: 6, order: 1, h: 2 }, + ], + }], + }); + + expect((await migration.up({ rootDir })).updated).toBe(1); + const grid = readJson(layoutsPath).layouts[0].grid; + expect(grid.every((g) => g.y === undefined)).toBe(true); + expect(grid.map((g) => g.order)).toEqual([0, 1]); + }); + + // The read path in `sequenceGrid` treats any grid carrying `order` as + // new-shape. The migration has to probe the same way, or converting a file + // would reorder it relative to how the server was already serving it. + it('keeps a real `order` when a stale `y` disagrees with it', async () => { + writeJson(layoutsPath, { + activeLayoutId: 'default', + layouts: [{ + id: 'default', name: 'Everything', builtIn: true, widgets: ['alpha', 'beta', 'gamma'], + grid: [ + // y says gamma is on top; order says it is last. order wins. + { id: 'gamma', x: 0, y: 0, w: 12, order: 2, h: 3 }, + { id: 'alpha', x: 0, y: 9, w: 6, order: 0, h: 4 }, + // No sequence of its own — appended by a legacy seeding migration. + { id: 'beta', x: 6, y: 0, w: 6, h: 2 }, + ], + }], + }); + + expect((await migration.up({ rootDir })).updated).toBe(1); + const grid = readJson(layoutsPath).layouts[0].grid; + expect(grid.map((g) => g.id)).toEqual(['alpha', 'gamma', 'beta']); + expect(grid.map((g) => g.order)).toEqual([0, 1, 2]); + }); + + // The read path dedupes by id keeping the first entry in FILE order, so the + // conversion has to as well — deduping after the sort would hand the widget + // a different rectangle than the server was already serving for it. + it('keeps the first duplicate in file order, not the topmost one', async () => { + writeJson(layoutsPath, { + activeLayoutId: 'default', + layouts: [{ + id: 'default', name: 'Everything', builtIn: true, widgets: ['alpha'], + grid: [ + { id: 'alpha', x: 2, y: 10, w: 4, h: 7 }, + { id: 'alpha', x: 0, y: 0, w: 12, h: 1 }, + ], + }], + }); + + expect((await migration.up({ rootDir })).updated).toBe(1); + expect(readJson(layoutsPath).layouts[0].grid) + .toEqual([{ id: 'alpha', x: 2, w: 4, order: 0, h: 7 }]); + }); + + // The read path clamps `y` to GRID_LEGACY_Y_MAX (200) before it ranks, so + // two out-of-range entries land on the same ceiling and are separated by + // column. Ranking the raw values here would order them the other way and + // the conversion would change a layout the server was already serving. + it('clamps an out-of-range legacy y the way the read path does', async () => { + writeJson(layoutsPath, { + activeLayoutId: 'default', + layouts: [{ + id: 'default', name: 'Everything', builtIn: true, widgets: ['alpha', 'beta'], + grid: [ + { id: 'alpha', x: 0, y: 500, w: 6, h: 2 }, + { id: 'beta', x: 6, y: 300, w: 6, h: 2 }, + ], + }], + }); + + expect((await migration.up({ rootDir })).updated).toBe(1); + // Both clamp to y 200, so the tie falls to column and alpha (x 0) leads. + // Ranking the raw values would have put beta (y 300) first. + expect(readJson(layoutsPath).layouts[0].grid.map((g) => g.id)).toEqual(['alpha', 'beta']); + }); + + it('converts every layout in the file, and leaves empty grids alone', async () => { + writeJson(layoutsPath, { + activeLayoutId: 'focus', + layouts: [ + { id: 'focus', name: 'Focus', builtIn: true, widgets: ['alpha'], grid: [{ id: 'alpha', x: 0, y: 0, w: 6, h: 4 }] }, + { id: 'custom', name: 'Custom', builtIn: false, widgets: ['beta'], grid: [{ id: 'beta', x: 3, y: 2, w: 3, h: 1 }] }, + { id: 'bare', name: 'Bare', builtIn: false, widgets: ['gamma'], grid: [] }, + ], + }); + + expect((await migration.up({ rootDir })).updated).toBe(2); + const after = readJson(layoutsPath); + expect(after.layouts.find((l) => l.id === 'bare').grid).toEqual([]); + expect(after.layouts.find((l) => l.id === 'custom').grid) + .toEqual([{ id: 'beta', x: 3, w: 3, order: 0, h: 1 }]); + }); +}); diff --git a/server/routes/dashboardLayouts.js b/server/routes/dashboardLayouts.js index ad17c6442a..e3953ad957 100644 --- a/server/routes/dashboardLayouts.js +++ b/server/routes/dashboardLayouts.js @@ -17,17 +17,25 @@ const router = Router(); // at the API boundary agree by construction. const idSchema = z.string().trim().min(1).max(svc.ID_MAX_LENGTH).regex(svc.ID_PATTERN, 'id must be lowercase kebab'); -// Grid items carry per-widget x/y/w/h so the dashboard can render a -// free-form layout. Bounds mirror the service-layer sanitizeGridItem so -// reads and writes agree by construction. The route enforces structural -// validity here; cross-field invariants (id ∈ widgets, dedup, x+w ≤ cols) -// are enforced by the .refine() on layoutSchema below and the service. +// Grid items carry per-widget x/w columns plus an `order` (the reading and +// packing sequence — the dashboard's only vertical coordinate). Bounds mirror +// the service-layer sanitizeGridItem so reads and writes agree by +// construction. The route enforces structural validity here; cross-field +// invariants (id ∈ widgets, dedup, x+w ≤ cols) are enforced by the .refine() +// on layoutSchema below and the service. const gridItemSchema = z.object({ id: z.string().trim().min(1).max(svc.WIDGET_ID_MAX_LENGTH), x: z.number().int().min(0).max(svc.GRID_COLS - 1), - y: z.number().int().min(0).max(svc.GRID_ROW_MAX), w: z.number().int().min(1).max(svc.GRID_COLS), - h: z.number().int().min(1).max(svc.GRID_ITEM_H_MAX), + // Optional so a client that only knows the legacy `y` shape can still save; + // the service resolves whichever is present into a dense sequence. + order: z.number().int().min(0).max(svc.GRID_ORDER_MAX).optional(), + // Legacy row position, accepted from pre-#4133 clients and converted on + // read. Never written back. + y: z.number().int().min(0).max(svc.GRID_LEGACY_Y_MAX).optional(), + // First-paint / older-client fallback height in rows. Optional because the + // rendered height comes from measuring the widget, not from this. + h: z.number().int().min(1).max(svc.GRID_ITEM_H_MAX).optional(), // Set when the user pinned this cell's height by dragging it. Absent means // the dashboard sizes the cell to its content (and treats `h` as the // first-paint fallback) — see DashboardGrid.jsx. diff --git a/server/routes/dashboardLayouts.test.js b/server/routes/dashboardLayouts.test.js index 4b5f8db59e..6938e85b9c 100644 --- a/server/routes/dashboardLayouts.test.js +++ b/server/routes/dashboardLayouts.test.js @@ -12,7 +12,8 @@ vi.mock('../services/dashboardLayouts.js', () => ({ WIDGETS_MAX: 50, WIDGET_ID_MAX_LENGTH: 80, GRID_COLS: 12, - GRID_ROW_MAX: 200, + GRID_ORDER_MAX: 49, + GRID_LEGACY_Y_MAX: 200, GRID_ITEM_H_MAX: 50, TIME_STRING_RE: /^([01]\d|2[0-3]):[0-5]\d$/, getState: vi.fn(), @@ -115,8 +116,8 @@ describe('PUT /api/dashboard/layouts/:id', () => { name: 'Custom', widgets: ['apps', 'cos'], grid: [ - { id: 'apps', x: 0, y: 0, w: 12, h: 4 }, - { id: 'cos', x: 0, y: 4, w: 6, h: 3 }, + { id: 'apps', x: 0, w: 12, order: 0, h: 4 }, + { id: 'cos', x: 0, w: 6, order: 1, h: 3 }, ], }); expect(res.status).toBe(200); @@ -125,19 +126,36 @@ describe('PUT /api/dashboard/layouts/:id', () => { name: 'Custom', widgets: ['apps', 'cos'], grid: [ - { id: 'apps', x: 0, y: 0, w: 12, h: 4 }, - { id: 'cos', x: 0, y: 4, w: 6, h: 3 }, + { id: 'apps', x: 0, w: 12, order: 0, h: 4 }, + { id: 'cos', x: 0, w: 6, order: 1, h: 3 }, ], }); }); + // A stale client bundle still posts the pre-#4133 `{ x, y, w, h }` shape; + // the route has to let it through so the service can convert it rather than + // 400-ing the user's save. + it('accepts a legacy grid item that carries `y` instead of `order`', async () => { + svc.saveLayout.mockResolvedValue({ activeLayoutId: 'default', layouts: [] }); + const res = await request(makeApp()) + .put('/api/dashboard/layouts/my-custom') + .send({ + name: 'Custom', + widgets: ['apps'], + grid: [{ id: 'apps', x: 0, y: 4, w: 12, h: 4 }], + }); + expect(res.status).toBe(200); + expect(svc.saveLayout.mock.calls[0][0].grid) + .toEqual([{ id: 'apps', x: 0, y: 4, w: 12, h: 4 }]); + }); + it('rejects grid items that reference unknown widgets', async () => { const res = await request(makeApp()) .put('/api/dashboard/layouts/my-custom') .send({ name: 'Custom', widgets: ['apps'], - grid: [{ id: 'apps', x: 0, y: 0, w: 4, h: 4 }, { id: 'ghost', x: 4, y: 0, w: 4, h: 4 }], + grid: [{ id: 'apps', x: 0, w: 4, order: 0, h: 4 }, { id: 'ghost', x: 4, w: 4, order: 1, h: 4 }], }); expect(res.status).toBe(400); expect(svc.saveLayout).not.toHaveBeenCalled(); @@ -149,7 +167,7 @@ describe('PUT /api/dashboard/layouts/:id', () => { .send({ name: 'Custom', widgets: ['apps'], - grid: [{ id: 'apps', x: 6, y: 0, w: 8, h: 4 }], + grid: [{ id: 'apps', x: 6, w: 8, order: 0, h: 4 }], }); expect(res.status).toBe(400); expect(svc.saveLayout).not.toHaveBeenCalled(); @@ -162,8 +180,8 @@ describe('PUT /api/dashboard/layouts/:id', () => { name: 'Custom', widgets: ['apps'], grid: [ - { id: 'apps', x: 0, y: 0, w: 4, h: 4 }, - { id: 'apps', x: 4, y: 0, w: 4, h: 4 }, + { id: 'apps', x: 0, w: 4, order: 0, h: 4 }, + { id: 'apps', x: 4, w: 4, order: 1, h: 4 }, ], }); expect(res.status).toBe(400); diff --git a/server/services/dashboardLayouts.js b/server/services/dashboardLayouts.js index 9c3e459b66..5b19b5e66d 100644 --- a/server/services/dashboardLayouts.js +++ b/server/services/dashboardLayouts.js @@ -28,12 +28,12 @@ const makeErr = (message, code) => Object.assign(new Error(message), { code }); // to an unknown id, the client skips it gracefully. // Built-in layouts ship with a `grid` so they look right out-of-the-box // instead of falling back to the client's row-flow synthesis. `x`/`w` are -// real: they place the widget in the 12 columns. `y` decides reading order — -// and therefore the order the client packs in — while `h` is only the +// real: they place the widget in the 12 columns. `order` decides reading +// order — and therefore the order the client packs in — while `h` is only the // first-paint height, because the renderer measures each widget and floats it -// up (see client/src/components/dashboard/DashboardGrid.jsx). So order these -// grids by what should be seen first; do NOT budget above-the-fold space by -// counting 80px rows, the arithmetic won't hold. +// up (see client/src/components/dashboard/DashboardGrid.jsx). There is no row +// coordinate: so order these grids by what should be seen first, and do NOT +// budget above-the-fold space by counting 80px rows, the arithmetic won't hold. // Intent-named layouts shipped post-029. Exported so the migration that // seeds them into existing installs (scripts/migrations/030-…) imports @@ -45,10 +45,10 @@ export const INTENT_LAYOUTS = [ name: 'Deep Work', widgets: ['quick-task', 'upcoming-tasks', 'cos', 'decision-log'], grid: [ - { id: 'quick-task', x: 0, y: 0, w: 6, h: 5 }, - { id: 'upcoming-tasks', x: 6, y: 0, w: 6, h: 10 }, - { id: 'cos', x: 0, y: 5, w: 6, h: 3 }, - { id: 'decision-log', x: 0, y: 8, w: 6, h: 3 }, + { id: 'quick-task', x: 0, w: 6, order: 0, h: 5 }, + { id: 'upcoming-tasks', x: 6, w: 6, order: 1, h: 10 }, + { id: 'cos', x: 0, w: 6, order: 2, h: 3 }, + { id: 'decision-log', x: 0, w: 6, order: 3, h: 3 }, ], }, { @@ -56,14 +56,14 @@ export const INTENT_LAYOUTS = [ name: 'Health', widgets: ['death-clock', 'goal-progress', 'activity-streak', 'daily-post', 'quick-brain', 'hourly-activity', 'meatspace-streak'], grid: [ - { id: 'death-clock', x: 0, y: 0, w: 4, h: 3 }, - { id: 'goal-progress', x: 4, y: 0, w: 5, h: 5 }, - { id: 'activity-streak', x: 9, y: 0, w: 3, h: 3 }, - { id: 'daily-post', x: 9, y: 3, w: 3, h: 2 }, - { id: 'quick-brain', x: 0, y: 3, w: 4, h: 2 }, - { id: 'hourly-activity', x: 0, y: 5, w: 12, h: 4 }, + { id: 'death-clock', x: 0, w: 4, order: 0, h: 3 }, + { id: 'goal-progress', x: 4, w: 5, order: 1, h: 5 }, + { id: 'activity-streak', x: 9, w: 3, order: 2, h: 3 }, + { id: 'quick-brain', x: 0, w: 4, order: 3, h: 2 }, + { id: 'daily-post', x: 9, w: 3, order: 4, h: 2 }, + { id: 'hourly-activity', x: 0, w: 12, order: 5, h: 4 }, // Gated on any health log existing — hidden on installs with no logs. - { id: 'meatspace-streak', x: 0, y: 9, w: 4, h: 4 }, + { id: 'meatspace-streak', x: 0, w: 4, order: 6, h: 4 }, ], }, { @@ -71,12 +71,12 @@ export const INTENT_LAYOUTS = [ name: 'Agent Watch', widgets: ['cos', 'proactive-alerts', 'review-hub', 'while-away', 'system-health', 'decision-log'], grid: [ - { id: 'cos', x: 0, y: 0, w: 6, h: 5 }, - { id: 'proactive-alerts', x: 6, y: 0, w: 3, h: 3 }, - { id: 'review-hub', x: 9, y: 0, w: 3, h: 3 }, - { id: 'while-away', x: 6, y: 3, w: 6, h: 5 }, - { id: 'system-health', x: 0, y: 5, w: 6, h: 5 }, - { id: 'decision-log', x: 6, y: 8, w: 6, h: 3 }, + { id: 'cos', x: 0, w: 6, order: 0, h: 5 }, + { id: 'proactive-alerts', x: 6, w: 3, order: 1, h: 3 }, + { id: 'review-hub', x: 9, w: 3, order: 2, h: 3 }, + { id: 'while-away', x: 6, w: 6, order: 3, h: 5 }, + { id: 'system-health', x: 0, w: 6, order: 4, h: 5 }, + { id: 'decision-log', x: 6, w: 6, order: 5, h: 3 }, ], }, ]; @@ -98,39 +98,37 @@ const DEFAULT_LAYOUTS = [ // forcing a "More options" click. Quick-brain stays small and // upcoming-tasks aligns with the taller capture cards. grid: [ - // Row 0–4: capture row + tasks - { id: 'quick-brain', x: 0, y: 0, w: 3, h: 2 }, - { id: 'quick-image', x: 0, y: 2, w: 3, h: 3 }, - { id: 'quick-task', x: 3, y: 0, w: 5, h: 5 }, - { id: 'upcoming-tasks', x: 8, y: 0, w: 4, h: 5 }, - // Row 5–9: primary monitoring + alerts - { id: 'system-health', x: 0, y: 5, w: 5, h: 5 }, - { id: 'proactive-alerts', x: 5, y: 5, w: 3, h: 3 }, - { id: 'death-clock', x: 8, y: 5, w: 4, h: 2 }, - { id: 'review-hub', x: 5, y: 8, w: 3, h: 2 }, - { id: 'activity-streak', x: 8, y: 7, w: 4, h: 3 }, - // Row 10–13: secondary widgets - { id: 'backup', x: 0, y: 10, w: 3, h: 4 }, - { id: 'quick-stats', x: 3, y: 10, w: 3, h: 3 }, - { id: 'goal-progress', x: 6, y: 10, w: 3, h: 4 }, - { id: 'network-exposure', x: 9, y: 10, w: 3, h: 5 }, - // Row 14–17: lower-priority + cos. while-away fills the x9–11 column - // in rows 15–17 — below network-exposure (ends at row 14) and above - // hourly-activity (starts at row 18), so it overlaps neither. - { id: 'decision-log', x: 0, y: 14, w: 4, h: 2 }, - { id: 'cos', x: 4, y: 14, w: 5, h: 4 }, - { id: 'while-away', x: 9, y: 15, w: 3, h: 3 }, - // Row 18+: full-width visualizations + apps - { id: 'hourly-activity', x: 0, y: 18, w: 12, h: 3 }, - { id: 'apps', x: 0, y: 21, w: 12, h: 8 }, - // Quick-idea (catalog) is positioned below apps so the seeded layout - // doesn't collide with the tightly-packed above-the-fold rows. + // Capture band + tasks + { id: 'quick-brain', x: 0, w: 3, order: 0, h: 2 }, + { id: 'quick-task', x: 3, w: 5, order: 1, h: 5 }, + { id: 'upcoming-tasks', x: 8, w: 4, order: 2, h: 5 }, + { id: 'quick-image', x: 0, w: 3, order: 3, h: 3 }, + // Primary monitoring + alerts + { id: 'system-health', x: 0, w: 5, order: 4, h: 5 }, + { id: 'proactive-alerts', x: 5, w: 3, order: 5, h: 3 }, + { id: 'death-clock', x: 8, w: 4, order: 6, h: 2 }, + { id: 'activity-streak', x: 8, w: 4, order: 7, h: 3 }, + { id: 'review-hub', x: 5, w: 3, order: 8, h: 2 }, + // Secondary widgets + { id: 'backup', x: 0, w: 3, order: 9, h: 4 }, + { id: 'quick-stats', x: 3, w: 3, order: 10, h: 3 }, + { id: 'goal-progress', x: 6, w: 3, order: 11, h: 4 }, + { id: 'network-exposure', x: 9, w: 3, order: 12, h: 5 }, + // Lower-priority + cos + { id: 'decision-log', x: 0, w: 4, order: 13, h: 2 }, + { id: 'cos', x: 4, w: 5, order: 14, h: 4 }, + { id: 'while-away', x: 9, w: 3, order: 15, h: 3 }, + // Full-width visualizations + apps + { id: 'hourly-activity', x: 0, w: 12, order: 16, h: 3 }, + { id: 'apps', x: 0, w: 12, order: 17, h: 8 }, + // Quick-idea (catalog) is sequenced below apps so the seeded layout + // doesn't crowd the tightly-packed above-the-fold band. // Reorderable via the Arrange button on the dashboard. - { id: 'quick-idea', x: 0, y: 29, w: 4, h: 4 }, + { id: 'quick-idea', x: 0, w: 4, order: 18, h: 4 }, // Gated on the Tribe having people — hidden on installs that don't use it. - { id: 'tribe-care', x: 4, y: 29, w: 4, h: 4 }, + { id: 'tribe-care', x: 4, w: 4, order: 19, h: 4 }, // Gated on having subscribed feeds — hidden on installs with none. - { id: 'feeds', x: 8, y: 29, w: 3, h: 4 }, + { id: 'feeds', x: 8, w: 3, order: 20, h: 4 }, ], }, { @@ -143,9 +141,9 @@ const DEFAULT_LAYOUTS = [ // upcoming-tasks tall on the right (the focus list); cos below // quick-task for streak/progress context. grid: [ - { id: 'quick-task', x: 0, y: 0, w: 6, h: 5 }, - { id: 'upcoming-tasks', x: 6, y: 0, w: 6, h: 10 }, - { id: 'cos', x: 0, y: 5, w: 6, h: 5 }, + { id: 'quick-task', x: 0, w: 6, order: 0, h: 5 }, + { id: 'upcoming-tasks', x: 6, w: 6, order: 1, h: 10 }, + { id: 'cos', x: 0, w: 6, order: 2, h: 5 }, ], }, { @@ -159,16 +157,16 @@ const DEFAULT_LAYOUTS = [ // review + goals fill the remaining quadrants. The Daily Driver (#2666) — the // first-visit-of-day sequence (POST → goal next-actions) — sits full-width in // a fresh row BELOW the scan quadrants: it self-hides once handled, and a - // gated-off widget's grid slot is dropped without compacting the rows above - // it, so placing it last (like the gated tribe-care/feeds widgets) means its - // absence leaves only harmless trailing space instead of a gap at the top. + // gated-off widget drops out of the sequence without disturbing what came + // before it, so sequencing it last (like the gated tribe-care/feeds + // widgets) means its absence leaves only harmless trailing space. grid: [ - { id: 'proactive-alerts', x: 0, y: 0, w: 4, h: 4 }, - { id: 'upcoming-tasks', x: 4, y: 0, w: 5, h: 8 }, - { id: 'death-clock', x: 9, y: 0, w: 3, h: 2 }, - { id: 'goal-progress', x: 9, y: 2, w: 3, h: 4 }, - { id: 'review-hub', x: 0, y: 4, w: 4, h: 4 }, - { id: 'daily-driver', x: 0, y: 8, w: 12, h: 6 }, + { id: 'proactive-alerts', x: 0, w: 4, order: 0, h: 4 }, + { id: 'upcoming-tasks', x: 4, w: 5, order: 1, h: 8 }, + { id: 'death-clock', x: 9, w: 3, order: 2, h: 2 }, + { id: 'goal-progress', x: 9, w: 3, order: 3, h: 4 }, + { id: 'review-hub', x: 0, w: 4, order: 4, h: 4 }, + { id: 'daily-driver', x: 0, w: 12, order: 5, h: 6 }, ], }, { @@ -181,12 +179,12 @@ const DEFAULT_LAYOUTS = [ // status, backup + quick-stats stacked on the right, apps grid fills // the empty cell below cos so all 5 widgets fit above the fold. grid: [ - { id: 'system-health', x: 0, y: 0, w: 6, h: 5 }, - { id: 'quick-stats', x: 6, y: 0, w: 6, h: 3 }, - { id: 'cos', x: 6, y: 3, w: 6, h: 4 }, - { id: 'backup', x: 0, y: 5, w: 3, h: 3 }, - { id: 'network-exposure', x: 3, y: 5, w: 3, h: 5 }, - { id: 'apps', x: 0, y: 10, w: 12, h: 11 }, + { id: 'system-health', x: 0, w: 6, order: 0, h: 5 }, + { id: 'quick-stats', x: 6, w: 6, order: 1, h: 3 }, + { id: 'cos', x: 6, w: 6, order: 2, h: 4 }, + { id: 'backup', x: 0, w: 3, order: 3, h: 3 }, + { id: 'network-exposure', x: 3, w: 3, order: 4, h: 5 }, + { id: 'apps', x: 0, w: 12, order: 5, h: 11 }, ], }, ...INTENT_LAYOUTS.map((l) => ({ ...l, builtIn: true })), @@ -208,15 +206,18 @@ export const NAME_MAX_LENGTH = 80; export const WIDGETS_MAX = 50; export const WIDGET_ID_MAX_LENGTH = 80; -// Grid placement bounds. The dashboard is a 12-column responsive grid; rows -// are integer steps (each ~80px tall). GRID_ROW_MAX caps total layout depth so -// a hand-edited file can't push `y` to absurd values — the renderer sorts by -// it, so a runaway value is a nonsense ordering rather than a nonsense -// position, but it stays clamped for the same reason `h` does: an older -// client still reads both as literal geometry. +// Grid placement bounds. The dashboard is a 12-column responsive grid whose +// vertical placement is packed from measured content heights, so `x`/`w` are +// the only real coordinates. `order` is the reading/packing sequence, bounded +// by the widget count because a layout can never hold more entries than that. +// `h` stays bounded because a pinned cell renders at exactly that many rows. export const GRID_COLS = 12; -export const GRID_ROW_MAX = 200; +export const GRID_ORDER_MAX = WIDGETS_MAX - 1; export const GRID_ITEM_H_MAX = 50; +// Legacy `y` (pre-#4133 layouts and stale client bundles) is still accepted on +// read and converted to `order`; this bounds how far a hand-edited value can +// reach before it is discarded and resequenced. +export const GRID_LEGACY_Y_MAX = 200; // Time-window auto-activation: HH:MM strings (24h). When a layout carries an // activateWindow and the local clock falls inside it, the dashboard @@ -239,28 +240,70 @@ const sanitizeActivateWindow = (w) => { return { start: w.start, end: w.end }; }; +const intOr = (v, fallback) => (Number.isFinite(v) ? Math.floor(v) : fallback); + // Clamp a single grid item to valid bounds. Returns null when the entry is // unusable (missing id, non-numeric coords, etc.). Numeric fields are // floored before clamping so JSON containing decimals can't smuggle in // off-grid positions that break the snap math in the client renderer. // +// The sequence field (`order`) is NOT resolved here — it's a property of the +// grid as a whole, so `sequenceGrid` below assigns it once every entry has +// been vetted and deduped. +// // `fixedH` marks a cell whose height the user pinned by dragging it. Absent // (the default) means the client sizes the cell to its content and floats it // up, using `h` only as the first-paint fallback — which is also what a -// client too old to know about `fixedH` renders. Emitted only when true so a -// hand-read layouts file stays terse. +// client too old to know about `fixedH` renders. Normalized to a plain boolean +// here for the intermediate entry; `sequenceGrid` is what drops it when false, +// so a hand-read layouts file stays terse. const sanitizeGridItem = (g, validIds) => { if (!g || typeof g !== 'object') return null; if (typeof g.id !== 'string') return null; const id = g.id.trim(); if (!id || !validIds.has(id)) return null; - const numOr = (v, fallback) => (Number.isFinite(v) ? Math.floor(v) : fallback); - const x = Math.max(0, Math.min(GRID_COLS - 1, numOr(g.x, 0))); - const y = Math.max(0, Math.min(GRID_ROW_MAX, numOr(g.y, 0))); - const wRaw = Math.max(1, Math.min(GRID_COLS, numOr(g.w, 1))); + const x = Math.max(0, Math.min(GRID_COLS - 1, intOr(g.x, 0))); + const wRaw = Math.max(1, Math.min(GRID_COLS, intOr(g.w, 1))); const w = Math.min(wRaw, GRID_COLS - x); - const h = Math.max(1, Math.min(GRID_ITEM_H_MAX, numOr(g.h, 1))); - return { id, x, y, w, h, ...(g.fixedH === true ? { fixedH: true } : {}) }; + const h = Math.max(1, Math.min(GRID_ITEM_H_MAX, intOr(g.h, 1))); + return { id, x, w, h, fixedH: g.fixedH === true }; +}; + +// Resolve the layout's reading sequence and emit dense `order` values. +// +// Version-gated by SHAPE rather than by a stored version number, so a layout +// that predates the conversion — an install that hasn't run migration 269, a +// restored backup, a save posted by a stale client bundle — still yields +// usable geometry instead of collapsing into one arbitrary order: +// - New shape: entries carry `order`; that's the sequence. +// - Legacy shape: entries carry a row position `y`, and the sequence is the +// reading order it implied — top-to-bottom, then left-to-right. +// - Mixed (a widget-seeding migration appending a legacy entry to an +// already-converted file) treats the layout as new-shape and puts the +// order-less entries last in file order, which is where those migrations +// mean to append. +// Output is always renumbered 0..n-1, so gaps and duplicates in the input +// can't survive a read. This is also the one place the emitted item shape is +// built, so `fixedH: false` is dropped rather than persisted. +const sequenceGrid = (entries) => { + const anyOrder = entries.some((e) => e.order !== null); + const ranked = entries.map((e, idx) => ({ + ...e, + idx, + rank: anyOrder ? (e.order ?? Number.MAX_SAFE_INTEGER) : (e.y ?? 0), + // Ties in legacy layouts are side-by-side cells: column decides. Ties in + // the new shape are corrupt data: file order decides. + tie: anyOrder ? idx : e.item.x, + })); + ranked.sort((a, b) => a.rank - b.rank || a.tie - b.tie || a.idx - b.idx); + return ranked.map(({ item }, order) => ({ + id: item.id, + x: item.x, + w: item.w, + order, + h: item.h, + ...(item.fixedH ? { fixedH: true } : {}), + })); }; // Sanitize a single layout entry — protect against hand-edits that produce @@ -292,7 +335,7 @@ const sanitizeLayout = (l) => { // grid entry without a matching widget is dead data and would render // nothing. Dedup by id so two entries can't both claim the same widget. const validIds = new Set(widgets); - const grid = []; + const entries = []; const seenGrid = new Set(); if (Array.isArray(l.grid)) { for (const g of l.grid) { @@ -300,9 +343,17 @@ const sanitizeLayout = (l) => { if (!item) continue; if (seenGrid.has(item.id)) continue; seenGrid.add(item.id); - grid.push(item); + entries.push({ + item, + // `null` = "this entry declares no sequence", distinct from a + // legitimate 0 — the shape probe in sequenceGrid depends on telling + // those apart. + order: Number.isFinite(g.order) ? Math.max(0, Math.min(GRID_ORDER_MAX, Math.floor(g.order))) : null, + y: Number.isFinite(g.y) ? Math.max(0, Math.min(GRID_LEGACY_Y_MAX, Math.floor(g.y))) : null, + }); } } + const grid = sequenceGrid(entries); const activateWindow = sanitizeActivateWindow(l.activateWindow); return { id: l.id, name, builtIn: BUILTIN_IDS.has(l.id), widgets, grid, activateWindow }; }; @@ -315,7 +366,7 @@ export const LIMITS = Object.freeze({ widgetsMax: WIDGETS_MAX, widgetIdMaxLength: WIDGET_ID_MAX_LENGTH, gridCols: GRID_COLS, - gridRowMax: GRID_ROW_MAX, + gridOrderMax: GRID_ORDER_MAX, gridItemHeightMax: GRID_ITEM_H_MAX, }); diff --git a/server/services/dashboardLayouts.test.js b/server/services/dashboardLayouts.test.js index 271b1909e2..7a8a2ee5ba 100644 --- a/server/services/dashboardLayouts.test.js +++ b/server/services/dashboardLayouts.test.js @@ -209,30 +209,93 @@ describe('dashboardLayouts service', () => { }); }); - describe('built-in layout grids have no overlapping cells', () => { - // A hand-tuned grid edit can place two widgets in the same rows/cols. The - // renderer no longer stacks them — it packs measured heights and resolves - // any declared overlap — but `y` is what it sorts reading order by, so an - // overlap means two widgets with an arbitrary relative order that will - // flip on an unrelated edit. Keep every built-in's rectangles pairwise - // non-overlapping so the seeded order is the one that was intended. - const rectsOverlap = (a, b) => - a.x < b.x + b.w && b.x < a.x + a.w && a.y < b.y + b.h && b.y < a.y + a.h; - - it('no two grid items overlap in any seeded built-in layout', async () => { + describe('built-in layout grids declare one coherent sequence', () => { + // `order` is the only vertical coordinate the renderer has: it decides + // both reading order and the order the pixel pack places cells in. A + // duplicate or a gap means two widgets with an arbitrary relative + // position that will flip on an unrelated edit, so every seeded built-in + // has to arrive as a dense 0..n-1 run — and inside the columns, which are + // still declared, no cell may hang off the right edge. + it('every seeded built-in layout has a dense, unique order', async () => { const state = await svc.getState(); - const collisions = []; for (const layout of state.layouts) { - const grid = layout.grid || []; - for (let i = 0; i < grid.length; i += 1) { - for (let j = i + 1; j < grid.length; j += 1) { - if (rectsOverlap(grid[i], grid[j])) { - collisions.push(`${layout.id}: ${grid[i].id} ∩ ${grid[j].id}`); - } - } + const orders = (layout.grid || []).map((g) => g.order); + expect({ id: layout.id, orders }) + .toEqual({ id: layout.id, orders: orders.map((_, i) => i) }); + } + }); + + it('no seeded built-in cell overflows the column count', async () => { + const state = await svc.getState(); + const overflow = []; + for (const layout of state.layouts) { + for (const g of layout.grid || []) { + if (g.x + g.w > svc.GRID_COLS) overflow.push(`${layout.id}: ${g.id}`); } } - expect(collisions).toEqual([]); + expect(overflow).toEqual([]); + }); + }); + + describe('grid shape compatibility', () => { + // An install that has not run migration 269 (or a restored pre-#4133 + // backup) still has to open in the arrangement its owner last saw: + // reading order was top-to-bottom, then left-to-right. + it('derives `order` from a legacy y/x grid, and stops emitting y', async () => { + writeJson(STATE_FILE, { + activeLayoutId: 'my-custom', + layouts: [{ + id: 'my-custom', name: 'Custom', widgets: ['cos', 'backup', 'apps'], + grid: [ + { id: 'apps', x: 0, y: 5, w: 12, h: 3 }, + { id: 'backup', x: 6, y: 0, w: 6, h: 2 }, + { id: 'cos', x: 0, y: 0, w: 6, h: 4 }, + ], + }], + }); + const saved = (await svc.getState()).layouts.find((l) => l.id === 'my-custom'); + expect(saved.grid).toEqual([ + { id: 'cos', x: 0, w: 6, order: 0, h: 4 }, + { id: 'backup', x: 6, w: 6, order: 1, h: 2 }, + { id: 'apps', x: 0, w: 12, order: 2, h: 3 }, + ]); + }); + + // A widget-seeding migration appends `{ x, y, w, h }` to a file this + // install already converted. Treating that as "y:0, so it goes first" + // would silently hoist the newcomer to the top of the dashboard. + it('appends an order-less entry last rather than letting its y win', async () => { + writeJson(STATE_FILE, { + activeLayoutId: 'my-custom', + layouts: [{ + id: 'my-custom', name: 'Custom', widgets: ['cos', 'backup'], + grid: [ + { id: 'cos', x: 0, w: 6, order: 0, h: 4 }, + { id: 'backup', x: 0, y: 0, w: 6, h: 2 }, + ], + }], + }); + const saved = (await svc.getState()).layouts.find((l) => l.id === 'my-custom'); + expect(saved.grid.map((g) => g.id)).toEqual(['cos', 'backup']); + expect(saved.grid.map((g) => g.order)).toEqual([0, 1]); + }); + + it('renumbers a hand-edited grid with duplicate and gapped orders', async () => { + writeJson(STATE_FILE, { + activeLayoutId: 'my-custom', + layouts: [{ + id: 'my-custom', name: 'Custom', widgets: ['cos', 'backup', 'apps'], + grid: [ + { id: 'cos', x: 0, w: 6, order: 9, h: 4 }, + { id: 'backup', x: 6, w: 6, order: 2, h: 2 }, + { id: 'apps', x: 0, w: 12, order: 2, h: 3 }, + ], + }], + }); + const saved = (await svc.getState()).layouts.find((l) => l.id === 'my-custom'); + // Ties in the new shape fall back to file order, not column. + expect(saved.grid.map((g) => g.id)).toEqual(['backup', 'apps', 'cos']); + expect(saved.grid.map((g) => g.order)).toEqual([0, 1, 2]); }); }); });