fix(pen-core): resolve fontSize/lineHeight/letterSpacing/cornerRadius $refs in resolveNodeForCanvas

P1.5 consumer compatibility gate before P2 element-tool v1 builders.

resolveNodeForCanvas was already resolving gap/padding/opacity/color refs,
but missed text typography fields (fontSize, lineHeight, letterSpacing) and
cornerRadius. Since both skia-engine and pen-renderer consume resolver output
before layout runs, unresolved string refs would pass arithmetic as NaN.

layout/engine.ts also patched to guard typeof === 'number' at the 4 text
font-size/line-height access points — defensive fallback to 16/1.x even when
called on pre-resolution nodes (e.g. MCP server, normalizer pipeline).

Adds 21-test p1-5-consumer-compat.test.ts covering all 6 consumer paths.
All 1183 pen-core tests pass. format:check and tsc --noEmit clean.
This commit is contained in:
Fini 2026-04-29 09:50:14 +08:00
parent 8a8151d35c
commit caa35e5d8b
4 changed files with 405 additions and 9 deletions

View file

@ -36,12 +36,14 @@ Multi-tool example — a "Notifications" settings section with a header + 4 togg
**`parent_id` is REAL or OMITTED — never invented.** Every `add_*_v0` tool's `parent_id` arg must either (1) be omitted entirely so the new node lands at the page root (the safe default for full-page composite briefs), or (2) name a real existing node id you already received from a prior tool call. The cookbook recipes below use placeholders like `<page>` / `<panel>` / `<sidebar>` as DOCUMENTATION shorthand only — when YOU emit the call in your output, OMIT the parent_id field entirely.
❌ WRONG — these all crash with `parent_id "X" not found in document` because every id was made up by the model, not handed to you by a prior tool call:
- `add_activity_log_v0({ parent_id: "entry-1", actor: "Sarah", ... })` — "entry-1" was never created
- `add_setting_row_v0({ parent_id: "root", title: "Email" })` — page root has no id
- `add_member_row_v0({ parent_id: "members-section", name: "Sarah" })` — section-* placeholders are docs, not real ids
- `add_member_row_v0({ parent_id: "members-section", name: "Sarah" })` — section-\* placeholders are docs, not real ids
- Same trap for any sequential name you might invent: `card-1` / `item-1` / `row-N` / `<page>` / `panel` / `canvas`
✅ RIGHT — omit `parent_id` entirely; each call lands at the page root, which is what every full-page composite brief wants:
- `add_activity_log_v0({ actor: "Sarah", action: "approved deploy", timestamp: "2h ago" })`
- `add_setting_row_v0({ title: "Email notifications", trailing: { kind: "switch", on: true } })`
- `add_member_row_v0({ name: "Sarah Lee", subtitle: "Designer" })`

View file

@ -0,0 +1,364 @@
/**
* P1.5 — Downstream consumer compatibility: $variable refs in numeric fields
*
* Verifies that all 6 consumer paths handle string $variable refs in numeric
* fields (fontSize, lineHeight, letterSpacing, cornerRadius, gap, padding)
* without crashing or silently producing wrong arithmetic.
*
* Consumers covered:
* 1. layout/engine.ts — getNodeWidth / computeLayoutPositions
* 2. layout/text-measure.ts — estimateTextHeight (safe by design)
* 3. variables/resolve.ts — resolveNodeForCanvas (root resolver, patched)
* 4. post-processing passes — normalize.ts + normalize-tree.ts (preserve refs)
* 5. skia-engine path — resolveNodeForCanvas → layout: resolver covers it
* 6. pen-renderer path — resolveNodeForCanvas → renderer: resolver covers it
*
* For consumers 5 & 6 (skia-engine / text-renderer): they both receive output of
* resolveNodeForCanvas, so their paths are covered by consumer 3 tests.
* pen-codegen has no in-tree source (npm-only package) — noted as N/A.
*/
import { describe, it, expect } from 'vitest';
import type { PenNode } from '@zseven-w/pen-types';
import type { VariableDefinition } from '@zseven-w/pen-types';
import { getNodeWidth, getNodeHeight, computeLayoutPositions } from '../layout/engine';
import { estimateTextHeight } from '../layout/text-measure';
import { resolveNodeForCanvas } from '../variables/resolve';
import { normalizePenDocument } from '../normalize';
import { normalizeTreeLayout } from '../layout/normalize-tree';
import { unwrapFakePhoneMockups } from '../layout/unwrap-fake-phone-mockup';
import { stripRedundantSectionFills } from '../layout/strip-redundant-section-fills';
// ---------------------------------------------------------------------------
// Shared fixtures
// ---------------------------------------------------------------------------
const VARS: Record<string, VariableDefinition> = {
'type-h1-size': { type: 'number', value: 24 },
'type-h1-line-height': { type: 'number', value: 1.2 },
'type-letter-spacing': { type: 'number', value: 0.5 },
'radius-md': { type: 'number', value: 8 },
'spacing-3': { type: 'number', value: 12 },
};
/** A text node with all key typography fields as $variable refs. */
const refTextNode = (): PenNode =>
({
id: 'txt',
type: 'text',
x: 0,
y: 0,
content: 'Hello World',
fontSize: '$type-h1-size' as unknown as number,
lineHeight: '$type-h1-line-height' as unknown as number,
letterSpacing: '$type-letter-spacing' as unknown as number,
}) as PenNode;
/** A frame with gap and cornerRadius as $variable refs. */
const refFrame = (children: PenNode[] = []): PenNode =>
({
id: 'frame',
type: 'frame',
x: 0,
y: 0,
width: 400,
height: 300,
layout: 'vertical',
gap: '$spacing-3' as unknown as number,
cornerRadius: '$radius-md' as unknown as number,
children,
}) as PenNode;
// ---------------------------------------------------------------------------
// Consumer 1: layout/engine.ts — getNodeWidth / getNodeHeight / computeLayoutPositions
// ---------------------------------------------------------------------------
describe('Consumer 1: layout/engine — ref-typed numeric fields', () => {
it('getNodeWidth with fontSize=$type-h1-size returns numeric (not NaN) — uses 16px defensive fallback', () => {
// The layout engine guards: typeof node.fontSize === 'number' ? node.fontSize : 16
// String ref → falls back to 16px. Result is numeric and > 0 (no crash, no NaN).
// The 24px estimate only arrives after resolveNodeForCanvas pre-processes the node.
const node = refTextNode();
const w = getNodeWidth(node);
expect(typeof w).toBe('number');
expect(isNaN(w)).toBe(false);
expect(w).toBeGreaterThan(0);
// The 16px fallback produces a smaller width than the 24px literal
const literalNode: PenNode = { ...node, fontSize: 24, letterSpacing: 0.5 } as PenNode;
const wLiteral = getNodeWidth(literalNode);
expect(wLiteral).toBeGreaterThan(w); // 24px wider than 16px
});
it('getNodeHeight with lineHeight=$type-h1-line-height is numeric — uses 16px defensive fallback', () => {
// Same as above: engine guards typeof, uses 16 fallback for string refs.
const node = refTextNode();
const h = getNodeHeight(node);
expect(typeof h).toBe('number');
expect(isNaN(h)).toBe(false);
expect(h).toBeGreaterThan(0);
// 24px literal produces taller result than 16px fallback
const literalNode: PenNode = { ...node, fontSize: 24, lineHeight: 1.2 } as PenNode;
expect(getNodeHeight(literalNode)).toBeGreaterThan(h);
});
it('computeLayoutPositions with text child using ref fontSize completes without NaN positions', () => {
const parent = refFrame([refTextNode()]);
const children = (parent as PenNode & { children: PenNode[] }).children;
const result = computeLayoutPositions(parent, children);
expect(result.length).toBeGreaterThan(0);
const positioned = result[0];
expect(isNaN(positioned.x as number)).toBe(false);
expect(isNaN(positioned.y as number)).toBe(false);
});
it('computeLayoutPositions with gap=$spacing-3 uses 12px gap', () => {
// gap is a string ref → typeof gap === 'string' → nodeGap = 0 fallback in engine
// This is intentional: engine treats string gap as 0 (conservative).
// After resolveNodeForCanvas resolves gap→12, engine sees numeric gap.
// This test documents the pre-resolution behavior (engine is defensive).
const parent: PenNode = {
id: 'f',
type: 'frame',
x: 0,
y: 0,
width: 300,
height: 300,
layout: 'vertical',
gap: '$spacing-3' as unknown as number,
children: [
{ id: 'a', type: 'rectangle', x: 0, y: 0, width: 50, height: 30 },
{ id: 'b', type: 'rectangle', x: 0, y: 0, width: 50, height: 30 },
],
} as PenNode;
const children = (parent as PenNode & { children: PenNode[] }).children;
// Should NOT crash — string gap is handled defensively (treated as 0)
expect(() => computeLayoutPositions(parent, children)).not.toThrow();
const result = computeLayoutPositions(parent, children);
expect(result.length).toBe(2);
// Second child starts at y=30 (no gap, since engine guards typeof gap === 'number')
expect(result[1].y).toBe(30);
});
});
// ---------------------------------------------------------------------------
// Consumer 2: layout/text-measure.ts — estimateTextHeight
// ---------------------------------------------------------------------------
describe('Consumer 2: layout/text-measure — estimateTextHeight with ref-typed fields', () => {
it('estimateTextHeight is safe when fontSize is a $ref string (uses 16px fallback)', () => {
const node = refTextNode();
// text-measure uses: typeof n.fontSize === 'number' ? n.fontSize : 16
// So string ref → falls back to 16, no crash, no NaN
const h = estimateTextHeight(node, 200);
expect(typeof h).toBe('number');
expect(isNaN(h)).toBe(false);
expect(h).toBeGreaterThan(0);
});
it('estimateTextHeight with literal fontSize=24 produces taller result than fallback 16', () => {
const node = refTextNode();
const hWithRef = estimateTextHeight(node, 200); // uses fallback 16
const literalNode: PenNode = { ...node, fontSize: 24, lineHeight: 1.2 } as PenNode;
const hLiteral = estimateTextHeight(literalNode, 200); // uses 24
// 24px text should be taller than 16px text
expect(hLiteral).toBeGreaterThan(hWithRef);
});
});
// ---------------------------------------------------------------------------
// Consumer 3: variables/resolve.ts — resolveNodeForCanvas (patched in P1.5)
// ---------------------------------------------------------------------------
describe('Consumer 3: resolveNodeForCanvas — resolves text/typography numeric refs', () => {
it('resolves fontSize=$type-h1-size → 24', () => {
const node = refTextNode();
const resolved = resolveNodeForCanvas(node, VARS);
const r = resolved as unknown as Record<string, unknown>;
expect(r.fontSize).toBe(24);
expect(typeof r.fontSize).toBe('number');
});
it('resolves lineHeight=$type-h1-line-height → 1.2', () => {
const node = refTextNode();
const resolved = resolveNodeForCanvas(node, VARS);
const r = resolved as unknown as Record<string, unknown>;
expect(r.lineHeight).toBe(1.2);
});
it('resolves letterSpacing=$type-letter-spacing → 0.5', () => {
const node = refTextNode();
const resolved = resolveNodeForCanvas(node, VARS);
const r = resolved as unknown as Record<string, unknown>;
expect(r.letterSpacing).toBe(0.5);
});
it('resolves cornerRadius=$radius-md → 8 on a frame', () => {
const frame = refFrame();
const resolved = resolveNodeForCanvas(frame, VARS);
const r = resolved as unknown as Record<string, unknown>;
expect(r.cornerRadius).toBe(8);
});
it('resolves all typography refs on a nested text node', () => {
const parent: PenNode = {
id: 'root',
type: 'frame',
x: 0,
y: 0,
width: 400,
height: 300,
children: [refTextNode()],
} as PenNode;
const resolved = resolveNodeForCanvas(parent, VARS);
const child = (resolved as PenNode & { children: PenNode[] }).children[0];
const c = child as unknown as Record<string, unknown>;
expect(c.fontSize).toBe(24);
expect(c.lineHeight).toBe(1.2);
expect(c.letterSpacing).toBe(0.5);
});
it('leaves already-numeric fontSize unchanged', () => {
const node: PenNode = { id: 'txt', type: 'text', x: 0, y: 0, content: 'Hi', fontSize: 18 };
const resolved = resolveNodeForCanvas(node, VARS);
expect((resolved as unknown as Record<string, unknown>).fontSize).toBe(18);
});
it('after resolution, no string $refs remain in fontSize/lineHeight/letterSpacing', () => {
const node = refTextNode();
const resolved = resolveNodeForCanvas(node, VARS);
const r = resolved as unknown as Record<string, unknown>;
// All resolved — no $refs remain
expect(typeof r.fontSize).toBe('number');
expect(typeof r.lineHeight).toBe('number');
expect(typeof r.letterSpacing).toBe('number');
});
it('layout engine receives correct sizes after resolveNodeForCanvas + computeLayoutPositions', () => {
// This simulates the skia-engine path:
// 1. resolveNodeForCanvas → 2. computeLayoutPositions
const rawParent = refFrame([refTextNode()]);
const resolvedParent = resolveNodeForCanvas(rawParent, VARS);
const children = (resolvedParent as PenNode & { children: PenNode[] }).children;
// After resolution, fontSize is 24
const resolvedText = children[0] as unknown as Record<string, unknown>;
expect(resolvedText.fontSize).toBe(24);
// Layout should compute correct text width/height
const positioned = computeLayoutPositions(resolvedParent, children);
expect(positioned.length).toBe(1);
expect(isNaN(positioned[0].x as number)).toBe(false);
expect(isNaN(positioned[0].y as number)).toBe(false);
// Width should match a 24px text estimate (not 16px fallback)
const w = (positioned[0] as unknown as Record<string, unknown>).width as number;
expect(typeof w).toBe('number');
expect(w).toBeGreaterThan(0);
});
});
// ---------------------------------------------------------------------------
// Consumer 4: post-processing passes — must not crash or strip refs
// ---------------------------------------------------------------------------
describe('Consumer 4: post-processing passes — preserve and not crash on ref-typed fields', () => {
it('normalizePenDocument preserves fontSize=$ref string', () => {
const doc = {
id: 'doc',
variables: VARS,
children: [refTextNode()],
} as any;
const normalized = normalizePenDocument(doc);
const child = normalized.children[0] as unknown as Record<string, unknown>;
expect(child.fontSize).toBe('$type-h1-size');
});
it('normalizePenDocument preserves cornerRadius=$ref string', () => {
const doc = {
id: 'doc',
variables: VARS,
children: [refFrame()],
} as any;
const normalized = normalizePenDocument(doc);
const child = normalized.children[0] as unknown as Record<string, unknown>;
expect(child.cornerRadius).toBe('$radius-md');
});
it('normalizeTreeLayout does not crash with fontSize=$ref on text nodes', () => {
// normalizeTreeLayout takes a single PenNode and mutates in place (returns void)
const root: PenNode = {
id: 'root',
type: 'frame',
x: 0,
y: 0,
width: 400,
height: 300,
layout: 'vertical',
children: [refTextNode()],
} as PenNode;
expect(() => normalizeTreeLayout(root)).not.toThrow();
// After normalization, ref strings should be preserved (not mutated)
const child = (root as PenNode & { children: PenNode[] }).children[0];
expect((child as unknown as Record<string, unknown>).fontSize).toBe('$type-h1-size');
});
it('unwrapFakePhoneMockups does not crash with cornerRadius=$ref', () => {
// Takes a single PenNode root and mutates in place
const root = refFrame([refTextNode()]);
expect(() => unwrapFakePhoneMockups(root)).not.toThrow();
});
it('stripRedundantSectionFills does not crash with ref-typed text nodes', () => {
// Takes a single PenNode root frame and mutates in place
const section: PenNode = {
id: 'section',
type: 'frame',
x: 0,
y: 0,
width: 400,
height: 300,
layout: 'vertical',
fill: [{ type: 'solid', color: '#1a1a1a' }],
children: [refTextNode()],
} as PenNode;
expect(() => stripRedundantSectionFills(section)).not.toThrow();
});
});
// ---------------------------------------------------------------------------
// Consumer 5 & 6: skia-engine / pen-renderer — resolveNodeForCanvas is the gate
// (No DOM / CanvasKit available in vitest; tested via resolver smoke tests above)
// ---------------------------------------------------------------------------
describe('Consumer 5+6: skia-engine and pen-renderer resolver smoke test', () => {
it('resolveNodeForCanvas returns only numeric fontSize/lineHeight — no string refs reach renderer', () => {
// Both skia-engine and pen-renderer consume output of resolveNodeForCanvas.
// If resolver produces only numeric values for these fields, renderer is safe.
const textWithRefs = refTextNode();
const resolved = resolveNodeForCanvas(textWithRefs, VARS);
const r = resolved as unknown as Record<string, unknown>;
// All three fields resolved to numbers — renderer path is safe
expect(typeof r.fontSize).toBe('number');
expect(typeof r.lineHeight).toBe('number');
expect(typeof r.letterSpacing).toBe('number');
// No $-prefixed strings in these fields
expect(String(r.fontSize).startsWith('$')).toBe(false);
expect(String(r.lineHeight).startsWith('$')).toBe(false);
expect(String(r.letterSpacing).startsWith('$')).toBe(false);
});
it('resolveNodeForCanvas with EMPTY vars leaves numeric fields as-is', () => {
// When no vars defined (early-return path), literal numbers pass through unchanged
const node: PenNode = {
id: 'txt',
type: 'text',
x: 0,
y: 0,
content: 'Hi',
fontSize: 20,
lineHeight: 1.4,
};
const resolved = resolveNodeForCanvas(node, {});
expect(resolved).toBe(node); // same reference — no allocation
});
});

View file

@ -190,8 +190,8 @@ export function getNodeWidth(node: PenNode, parentAvail?: number): number {
if (intrinsic > 0) return intrinsic;
}
if (node.type === 'text') {
const fontSize = node.fontSize ?? 16;
const letterSpacing = node.letterSpacing ?? 0;
const fontSize = typeof node.fontSize === 'number' ? node.fontSize : 16;
const letterSpacing = typeof node.letterSpacing === 'number' ? node.letterSpacing : 0;
const fontWeight = node.fontWeight;
const content = resolveTextContent(node);
return Math.max(
@ -210,8 +210,8 @@ export function getNodeWidth(node: PenNode, parentAvail?: number): number {
if (fit > 0) return fit;
}
if (node.type === 'text') {
const fontSize = node.fontSize ?? 16;
const letterSpacing = node.letterSpacing ?? 0;
const fontSize = typeof node.fontSize === 'number' ? node.fontSize : 16;
const letterSpacing = typeof node.letterSpacing === 'number' ? node.letterSpacing : 0;
const fontWeight = node.fontWeight;
const content = resolveTextContent(node);
return Math.max(
@ -296,8 +296,9 @@ export function computeLayoutPositions(parent: PenNode, children: PenNode[]): Pe
if (isVertical && ch.type === 'text' && mainSizing[i] !== 'fill') {
const content = resolveTextContent(ch);
if (countExplicitTextLines(content) <= 1) {
const fontSize = ch.fontSize ?? 16;
const lineHeight = ch.lineHeight ?? defaultLineHeight(fontSize);
const fontSize = typeof ch.fontSize === 'number' ? ch.fontSize : 16;
const lineHeight =
typeof ch.lineHeight === 'number' ? ch.lineHeight : defaultLineHeight(fontSize);
const singleLineH = fontSize * lineHeight;
const estH = estimateTextHeight(ch, availW);
if (estH <= singleLineH + 1) {
@ -347,8 +348,9 @@ export function computeLayoutPositions(parent: PenNode, children: PenNode[]): Pe
let effectiveChildCross = childCross;
if (align === 'center' && !isVertical && child.type === 'text') {
const fontSize = child.fontSize ?? 16;
const lineHeight = child.lineHeight ?? defaultLineHeight(fontSize);
const fontSize = typeof child.fontSize === 'number' ? child.fontSize : 16;
const lineHeight =
typeof child.lineHeight === 'number' ? child.lineHeight : defaultLineHeight(fontSize);
const content = resolveTextContent(child);
const isSingleLine = countExplicitTextLines(content) <= 1;
if (isSingleLine) {

View file

@ -273,6 +273,34 @@ export function resolveNodeForCanvas(node: PenNode, variables: Vars, activeTheme
}
}
// Typography numeric refs (text nodes only) — fontSize / lineHeight / letterSpacing
// These must be resolved before the layout engine and renderer consume them,
// since both paths use `node.fontSize ?? 16` (not type-guarded for strings).
if (node.type === 'text') {
const n = node as unknown as Record<string, unknown>;
for (const key of ['fontSize', 'lineHeight', 'letterSpacing'] as const) {
if (typeof n[key] === 'string' && isVariableRef(n[key] as string)) {
const resolved = resolveNumericRef(n[key] as string, variables, activeTheme);
if (resolved !== undefined) {
out[key] = resolved;
changed = true;
}
}
}
}
// cornerRadius scalar ref (frames, rectangles, etc.)
{
const n = node as unknown as Record<string, unknown>;
if (typeof n.cornerRadius === 'string' && isVariableRef(n.cornerRadius)) {
const resolved = resolveNumericRef(n.cornerRadius, variables, activeTheme);
if (resolved !== undefined) {
out.cornerRadius = resolved;
changed = true;
}
}
}
// Text content
if (node.type === 'text' && typeof node.content === 'string' && isVariableRef(node.content)) {
const resolved = resolveVariableRef(node.content, variables, activeTheme);