From 88f8b3ca2ec8ceb15e2ed9d2629d85fbc3e967c5 Mon Sep 17 00:00:00 2001 From: Ivo Date: Sun, 28 Jun 2026 22:32:58 +0200 Subject: [PATCH 1/2] Fix multi-column Row NaN (default cells to column count) + border width px unit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two type-checks-but-renders-broken footguns found by cold-start agent testing: - A multi-column with no `layout`/`cells` defaulted `cells` to `[1]` regardless of column count, so the 2nd/3rd had no cell and rendered width="NaN". Default to one equal cell PER child, matching renderToJson. (Also makes a `layout` accidentally placed on harmless — the Row no longer NaNs.) - A number border width (`borderTopWidth: 1`) rendered `border-top: 1 solid` (invalid CSS the browser drops), even though BorderInput + its JSDoc accept a number. Normalize border *Width fields (number / unit-less numeric string → px) in the mapper, for both nested `border` objects and gathered flat side props. +6 tests (shared mapper normalization + react render-level for both). Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/react/src/components/Row.tsx | 13 ++++- packages/react/src/dx-footguns.test.tsx | 50 +++++++++++++++++++ .../shared/src/utils/semantic-props.test.ts | 31 ++++++++++++ packages/shared/src/utils/semantic-props.ts | 14 ++++++ 4 files changed, 106 insertions(+), 2 deletions(-) diff --git a/packages/react/src/components/Row.tsx b/packages/react/src/components/Row.tsx index 8343ac7..df0fcd9 100644 --- a/packages/react/src/components/Row.tsx +++ b/packages/react/src/components/Row.tsx @@ -243,11 +243,20 @@ const Row: React.FC = (props) => { // Resolve mode: explicit prop > _config > default const mode: RenderMode = modeProp ?? _config?.mode ?? "web"; - // Determine cells from layout or props - let cells = propsCells || [1]; + // Determine cells from layout or props. With neither, default to one equal + // cell PER child (mirroring renderToJson) — `[1]` regardless of column + // count left the 2nd/3rd column with no cell, rendering width="NaN". + let cells: number[]; if (layout) { validateColumnLayout(layout, React.Children.count(children)); cells = layout.cells; + } else if (propsCells) { + cells = propsCells; + } else { + const columnCount = React.Children.toArray(children).filter( + (c) => React.isValidElement(c) && /^Column$/.test((c.type as any)?.displayName || (c.type as any)?.name || "") + ).length; + cells = Array(Math.max(1, columnCount)).fill(1); } // Merge body values with defaults diff --git a/packages/react/src/dx-footguns.test.tsx b/packages/react/src/dx-footguns.test.tsx index a24acb6..6c5a505 100644 --- a/packages/react/src/dx-footguns.test.tsx +++ b/packages/react/src/dx-footguns.test.tsx @@ -110,3 +110,53 @@ describe("attrs href round-trips into the editor (renderToJson stores values.hre expect(href.values.href).toBe("https://x.com/s"); }); }); + +describe("multi-column Row without an explicit layout defaults to equal columns (no NaN)", () => { + const threeColNoLayout = ( + + + + + + + + ); + it("renderToHtml emits no NaN width", () => { + expect(renderToHtml(threeColNoLayout)).not.toContain("NaN"); + }); + it("renderToJson defaults cells to the Column count", () => { + expect((renderToJson(threeColNoLayout) as any).body.rows[0].cells).toEqual([1, 1, 1]); + }); + it("a layout mistakenly placed on still doesn't NaN (Row defaults)", () => { + const html = renderToHtml( + + + + + ); + expect(html).not.toContain("NaN"); + }); +}); + +describe("border widths render with a px unit (number → Npx)", () => { + const borderTop = (border: any) => { + const html = renderToHtml( + + ); + return (html.match(/border-top:[^;"]*/i) || [""])[0]; + }; + it("a number border width gets px (not unitless)", () => { + expect(borderTop({ borderTopWidth: 1, borderTopStyle: "solid", borderTopColor: "#000" })).toBe("border-top: 1px solid #000"); + }); + it("a px-string border width is unchanged", () => { + expect(borderTop({ borderTopWidth: "3px", borderTopStyle: "solid", borderTopColor: "#000" })).toBe("border-top: 3px solid #000"); + }); + it("flat border-side props (gathered) also get px", () => { + const html = renderToHtml( + + + + ); + expect(html).toContain("border-bottom: 2px solid #abc"); + }); +}); diff --git a/packages/shared/src/utils/semantic-props.test.ts b/packages/shared/src/utils/semantic-props.test.ts index bf98ddc..2cd789a 100644 --- a/packages/shared/src/utils/semantic-props.test.ts +++ b/packages/shared/src/utils/semantic-props.test.ts @@ -405,3 +405,34 @@ describe("normalizeValuesForExporter", () => { expect(out.href).toEqual(RENDER_HREF); }); }); + +describe("border width px normalization", () => { + // Normalization only runs for components that declare a `border` field (Column, + // Divider, Table, …); use a Column-like defaults with an empty border group. + const COLUMN_DEFAULTS = { border: {}, padding: "0px" } as any; + + // The BorderInput type accepts a number, but a bare number renders + // `border-top: 1 solid` (invalid CSS); the mapper must add the px unit. + it("normalizes a number border width to px", () => { + const r = mapSemanticProps( + { border: { borderTopWidth: 1, borderTopStyle: "solid", borderTopColor: "#000" } }, + COLUMN_DEFAULTS, + "Column" + ); + expect((r.border as any).borderTopWidth).toBe("1px"); + }); + + it("gathers flat border-side props and px-normalizes the width", () => { + const r = mapSemanticProps( + { borderBottomWidth: 2, borderBottomStyle: "solid", borderBottomColor: "#abc" }, + COLUMN_DEFAULTS, + "Column" + ); + expect((r.border as any).borderBottomWidth).toBe("2px"); + }); + + it("leaves a px / non-numeric width string unchanged", () => { + const r = mapSemanticProps({ border: { borderTopWidth: "3px" } }, COLUMN_DEFAULTS, "Column"); + expect((r.border as any).borderTopWidth).toBe("3px"); + }); +}); diff --git a/packages/shared/src/utils/semantic-props.ts b/packages/shared/src/utils/semantic-props.ts index 51c706f..270045a 100644 --- a/packages/shared/src/utils/semantic-props.ts +++ b/packages/shared/src/utils/semantic-props.ts @@ -314,6 +314,20 @@ export function mapSemanticProps>( if (Object.keys(collected).length > 0) { final.border = { ...(final.border || {}), ...collected }; } + + // Border widths must carry a unit — `borderTopWidth: 1` would otherwise + // render `border-top: 1 solid` (invalid CSS the browser drops). The + // BorderInput type accepts a number, so normalize *Width fields (bare number + // or unit-less numeric string → px), wherever the border came from. + if (final.border && typeof final.border === "object") { + const b = final.border as Record; + for (const key of Object.keys(b)) { + if (!/Width$/.test(key)) continue; + const v = b[key]; + if (typeof v === "number") b[key] = `${v}px`; + else if (typeof v === "string" && /^\d+(?:\.\d+)?$/.test(v.trim())) b[key] = `${v.trim()}px`; + } + } } return final as T; From 8c566c90aa086bda12e5de6f81dafe13ef9c776f Mon Sep 17 00:00:00 2001 From: Ivo Date: Sun, 28 Jun 2026 22:38:59 +0200 Subject: [PATCH 2/2] Review: clone border before px-normalizing (keep mapSemanticProps pure) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The width normalization mutated final.border in place, but final.border can alias the caller's object (the values escape hatch is a shallow clone; a nested border prop passes by reference) — so a reused const HAIRLINE would be rewritten to '1px' as a side effect. Clone before rewriting, reassign. +purity test. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/shared/src/utils/semantic-props.test.ts | 8 ++++++++ packages/shared/src/utils/semantic-props.ts | 8 +++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/packages/shared/src/utils/semantic-props.test.ts b/packages/shared/src/utils/semantic-props.test.ts index 2cd789a..8db743e 100644 --- a/packages/shared/src/utils/semantic-props.test.ts +++ b/packages/shared/src/utils/semantic-props.test.ts @@ -435,4 +435,12 @@ describe("border width px normalization", () => { const r = mapSemanticProps({ border: { borderTopWidth: "3px" } }, COLUMN_DEFAULTS, "Column"); expect((r.border as any).borderTopWidth).toBe("3px"); }); + + it("does not mutate the caller's border object (stays pure)", () => { + // A reused `const HAIRLINE` must not be rewritten as a side effect. + const userBorder = { borderTopWidth: 1, borderTopStyle: "solid", borderTopColor: "#000" }; + const r = mapSemanticProps({ border: userBorder }, COLUMN_DEFAULTS, "Column"); + expect((r.border as any).borderTopWidth).toBe("1px"); // output normalized + expect(userBorder.borderTopWidth).toBe(1); // caller's object untouched + }); }); diff --git a/packages/shared/src/utils/semantic-props.ts b/packages/shared/src/utils/semantic-props.ts index 270045a..1f8712c 100644 --- a/packages/shared/src/utils/semantic-props.ts +++ b/packages/shared/src/utils/semantic-props.ts @@ -320,13 +320,19 @@ export function mapSemanticProps>( // BorderInput type accepts a number, so normalize *Width fields (bare number // or unit-less numeric string → px), wherever the border came from. if (final.border && typeof final.border === "object") { - const b = final.border as Record; + // Clone before rewriting: final.border may alias the caller's object (the + // `values` escape hatch is only a shallow clone, and a nested `border` prop + // passes through by reference), so mutating in place would be an impure side + // effect — e.g. a shared `const HAIRLINE = { borderTopWidth: 1 }` would get + // rewritten to "1px" in the caller. + const b: Record = { ...(final.border as Record) }; for (const key of Object.keys(b)) { if (!/Width$/.test(key)) continue; const v = b[key]; if (typeof v === "number") b[key] = `${v}px`; else if (typeof v === "string" && /^\d+(?:\.\d+)?$/.test(v.trim())) b[key] = `${v.trim()}px`; } + final.border = b; } }