From 8562711f49103a9c5d62785ff055487c4b214017 Mon Sep 17 00:00:00 2001 From: saravanakumardb1 Date: Wed, 27 May 2026 18:46:19 -0700 Subject: [PATCH] fix(workspace+theme): sanitise corrupt spans, short-circuit re-reconcile, drop unreachable regex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ────────────────────────────────────────────────────────────────── customizable-workspace ────────────────────────────────────────────────────────────────── Two issues caught in the audit pass: 1. **Corrupt persisted spans broke the grid layout.** localStorage entries from older versions (or hand-edited debug sessions) could contain span=NaN / 0 / -3 / 99. These flowed straight into `grid-column: span ` which silently broke the whole row. The visual symptom was a tile rendering at zero width or pushing every sibling off-screen. Fix: `reconcile()` now clamps every span (including newly appended tiles' defaultSpan) to the legal `[1, 4]` range via `sanitiseSpan()`. 2. **Re-reconcile effect could loop when callers forget to memoise.** The `useEffect([tiles, hydrated])` always called `setLayout` with a fresh `{ entries: [...] }` object reference, even when the content was identical. If `tiles` itself was a fresh reference per parent render (e.g. `tiles=[{...}]` inline), every render \u2192 setLayout \u2192 save effect \u2192 (no loop because tiles ref same), but constant unnecessary writes to localStorage. Fix: added `sameLayout(a, b)` structural-equality check; setLayout now short-circuits to the previous state when the reconciled output is identical. Tests: 10 \u2192 11 reconcile \u00b7 sanitises corrupt spans (NaN/0/negative/>4) \u2192 clamp ────────────────────────────────────────────────────────────────── generative-theme ────────────────────────────────────────────────────────────────── Cosmetic but worth fixing: the `rose` palette regex included `warm` as a keyword, but the `citrus` palette \u2014 listed earlier in the PALETTES table \u2014 also matched `warm`. Since first-match wins, `warm` was unreachable in rose and the entry was misleading. Dropped `warm` from the rose regex. Citrus retains it (was always where it routed in practice). All 18 existing tests still pass. --- .../customizable-workspace/src/Workspace.tsx | 14 ++++---- .../src/__tests__/workspace.test.tsx | 18 ++++++++++ .../src/useWorkspaceLayout.ts | 35 ++++++++++++++++--- packages/generative-theme/src/generate.ts | 2 +- 4 files changed, 56 insertions(+), 13 deletions(-) diff --git a/packages/customizable-workspace/src/Workspace.tsx b/packages/customizable-workspace/src/Workspace.tsx index b47df528..b37bc8d7 100644 --- a/packages/customizable-workspace/src/Workspace.tsx +++ b/packages/customizable-workspace/src/Workspace.tsx @@ -52,7 +52,7 @@ export function Workspace({ const [dropIndex, setDropIndex] = useState(null); const onDragStart = useCallback( - (e: DragEvent, idx: number) => { + (e: DragEvent, idx: number) => { if (readOnly) return; setDragIndex(idx); e.dataTransfer.effectAllowed = 'move'; @@ -66,7 +66,7 @@ export function Workspace({ ); const onDragOver = useCallback( - (e: DragEvent, idx: number) => { + (e: DragEvent, idx: number) => { if (readOnly) return; e.preventDefault(); e.dataTransfer.dropEffect = 'move'; @@ -76,7 +76,7 @@ export function Workspace({ ); const onDrop = useCallback( - (e: DragEvent, idx: number) => { + (e: DragEvent, idx: number) => { if (readOnly) return; e.preventDefault(); const from = dragIndex; @@ -124,7 +124,7 @@ export function Workspace({ )}
{tile.content}
-
+ ); })} diff --git a/packages/customizable-workspace/src/__tests__/workspace.test.tsx b/packages/customizable-workspace/src/__tests__/workspace.test.tsx index 510d070a..8a2c15cc 100644 --- a/packages/customizable-workspace/src/__tests__/workspace.test.tsx +++ b/packages/customizable-workspace/src/__tests__/workspace.test.tsx @@ -35,6 +35,24 @@ describe('reconcile', () => { expect(out.entries.map((e) => e.id)).toEqual(['a']); }); + it('sanitises corrupt spans (NaN / 0 / negative / >4) to the legal [1, 4] range', () => { + const out = reconcile( + { entries: [ + { id: 'a', span: Number.NaN as unknown as 2 }, + { id: 'b', span: 0 as unknown as 2 }, + { id: 'c', span: -3 as unknown as 2 }, + { id: 'd', span: 99 as unknown as 2 }, + ] }, + [ + { id: 'a', title: 'A', content: null }, + { id: 'b', title: 'B', content: null }, + { id: 'c', title: 'C', content: null }, + { id: 'd', title: 'D', content: null }, + ], + ); + expect(out.entries.map((e) => e.span)).toEqual([2, 1, 1, 4]); + }); + it('appends new tiles in registry order with defaultSpan fallback', () => { const out = reconcile( { entries: [{ id: 'a', span: 3 }] }, diff --git a/packages/customizable-workspace/src/useWorkspaceLayout.ts b/packages/customizable-workspace/src/useWorkspaceLayout.ts index f3f7fbf5..ad990e62 100644 --- a/packages/customizable-workspace/src/useWorkspaceLayout.ts +++ b/packages/customizable-workspace/src/useWorkspaceLayout.ts @@ -76,9 +76,14 @@ export function useWorkspaceLayout( }, [options.storageKey]); // Re-reconcile whenever the tile registry changes (add/remove/reorder). + // Short-circuit identical layouts to avoid setLayout → save effect → + // re-render loop when callers forget to memoise `tiles`. useEffect(() => { if (!hydrated) return; - setLayout((cur) => reconcile(cur, tiles)); + setLayout((cur) => { + const next = reconcile(cur, tiles); + return sameLayout(cur, next) ? cur : next; + }); }, [tiles, hydrated]); // Persist on every change (after hydration so initial load doesn't write back). @@ -116,20 +121,40 @@ export function useWorkspaceLayout( /** * Drop entries that no longer exist; append new tiles in registry order * with their default span. Pure function (used by tests). + * + * Also sanitises spans — a corrupt persisted entry with span=NaN / + * 0 / negative / 100 used to render "grid-column: span NaN" which + * silently broke the layout; we now clamp to `[1, 4]`. */ export function reconcile( layout: LayoutSpec, tiles: TileSpec[], ): LayoutSpec { const knownIds = new Set(tiles.map((t) => t.id)); - const surviving: LayoutEntry[] = layout.entries.filter((e) => - knownIds.has(e.id), - ); + const surviving: LayoutEntry[] = layout.entries + .filter((e) => knownIds.has(e.id)) + .map((e) => ({ id: e.id, span: sanitiseSpan(e.span) })); const seen = new Set(surviving.map((e) => e.id)); for (const t of tiles) { if (!seen.has(t.id)) { - surviving.push({ id: t.id, span: t.defaultSpan ?? 2 }); + surviving.push({ id: t.id, span: sanitiseSpan(t.defaultSpan ?? 2) }); } } return { entries: surviving }; } + +function sanitiseSpan(span: unknown): TileSpan { + const n = typeof span === 'number' && Number.isFinite(span) ? span : 2; + return Math.max(1, Math.min(4, Math.round(n))) as TileSpan; +} + +/** Cheap structural equality between two layouts. */ +function sameLayout(a: LayoutSpec, b: LayoutSpec): boolean { + if (a.entries.length !== b.entries.length) return false; + for (let i = 0; i < a.entries.length; i++) { + const ae = a.entries[i]!; + const be = b.entries[i]!; + if (ae.id !== be.id || ae.span !== be.span) return false; + } + return true; +} diff --git a/packages/generative-theme/src/generate.ts b/packages/generative-theme/src/generate.ts index 8ae1a040..801d5efe 100644 --- a/packages/generative-theme/src/generate.ts +++ b/packages/generative-theme/src/generate.ts @@ -61,7 +61,7 @@ const PALETTES: Array<{ textSecondary: '#3a5d7d', }, { - test: /\b(rose|blossom|peach|pink|warm|romantic|gentle)\b/i, + test: /\b(rose|blossom|peach|pink|romantic|gentle)\b/i, name: 'rose', accent: '#ec4899', surface: '#fff5fa',