fix(workspace+theme): sanitise corrupt spans, short-circuit re-reconcile, drop unreachable regex
──────────────────────────────────────────────────────────────────
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 <bad>` 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.
This commit is contained in:
parent
b2e45380ec
commit
8562711f49
@ -52,7 +52,7 @@ export function Workspace({
|
||||
const [dropIndex, setDropIndex] = useState<number | null>(null);
|
||||
|
||||
const onDragStart = useCallback(
|
||||
(e: DragEvent<HTMLDivElement>, idx: number) => {
|
||||
(e: DragEvent<HTMLElement>, idx: number) => {
|
||||
if (readOnly) return;
|
||||
setDragIndex(idx);
|
||||
e.dataTransfer.effectAllowed = 'move';
|
||||
@ -66,7 +66,7 @@ export function Workspace({
|
||||
);
|
||||
|
||||
const onDragOver = useCallback(
|
||||
(e: DragEvent<HTMLDivElement>, idx: number) => {
|
||||
(e: DragEvent<HTMLElement>, idx: number) => {
|
||||
if (readOnly) return;
|
||||
e.preventDefault();
|
||||
e.dataTransfer.dropEffect = 'move';
|
||||
@ -76,7 +76,7 @@ export function Workspace({
|
||||
);
|
||||
|
||||
const onDrop = useCallback(
|
||||
(e: DragEvent<HTMLDivElement>, idx: number) => {
|
||||
(e: DragEvent<HTMLElement>, idx: number) => {
|
||||
if (readOnly) return;
|
||||
e.preventDefault();
|
||||
const from = dragIndex;
|
||||
@ -124,7 +124,7 @@ export function Workspace({
|
||||
</div>
|
||||
)}
|
||||
<div
|
||||
role="grid"
|
||||
role="region"
|
||||
aria-label="Workspace tiles"
|
||||
data-testid="bl-workspace-grid"
|
||||
style={{
|
||||
@ -139,9 +139,9 @@ export function Workspace({
|
||||
if (!tile) return null;
|
||||
const span = Math.max(1, Math.min(columns, entry.span)) as TileSpan;
|
||||
return (
|
||||
<div
|
||||
<article
|
||||
key={entry.id}
|
||||
role="gridcell"
|
||||
aria-label={tile.title}
|
||||
data-testid="bl-workspace-tile"
|
||||
data-tile-id={entry.id}
|
||||
data-drop-target={dropIndex === idx ? 'true' : undefined}
|
||||
@ -253,7 +253,7 @@ export function Workspace({
|
||||
)}
|
||||
</header>
|
||||
<div>{tile.content}</div>
|
||||
</div>
|
||||
</article>
|
||||
);
|
||||
})}
|
||||
</div>
|
||||
|
||||
@ -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 }] },
|
||||
|
||||
@ -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;
|
||||
}
|
||||
|
||||
@ -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',
|
||||
|
||||
Loading…
Reference in New Issue
Block a user