diff --git a/packages/pen-ai-skills/skills/phases/generation/layout.md b/packages/pen-ai-skills/skills/phases/generation/layout.md index dce37d6c6..87643c8f7 100644 --- a/packages/pen-ai-skills/skills/phases/generation/layout.md +++ b/packages/pen-ai-skills/skills/phases/generation/layout.md @@ -102,3 +102,10 @@ AESTHETIC HYGIENE — keep these silent (never emit, the post-pass also strips t - INNER LAYOUT FRAMES (sections, wrappers, header / body containers inside a card) DO NOT need fill, stroke, OR shadow. They inherit from the page / card surface. Only opt into a fill / border / shadow on the OUTER card, button, badge, chip — NEVER on the wrapper that holds it. +- PAGE GUTTER GOES ON ONE LAYER, NOT BOTH. Pick: either the root frame carries horizontal padding + (e.g. `padding: [0,16]` on root) and direct child sections use horizontal padding 0, OR the root + carries 0 horizontal padding and each section sets its own. Stacking both produces a doubled + inset (root 16 + section 24 = 40px gutter on a 375px page → only 295px content). Default + convention: PUT HORIZONTAL PADDING ON THE ROOT, sections set vertical padding only. Hero / + banner / image-bleed sections then sit edge-to-edge by simply NOT adding horizontal padding (the + root's gutter shows through them naturally). diff --git a/packages/pen-ai-skills/src/__tests__/detectors-spacing.test.ts b/packages/pen-ai-skills/src/__tests__/detectors-spacing.test.ts index 081db2ab8..5a0260745 100644 --- a/packages/pen-ai-skills/src/__tests__/detectors-spacing.test.ts +++ b/packages/pen-ai-skills/src/__tests__/detectors-spacing.test.ts @@ -1,6 +1,9 @@ import { describe, it, expect } from 'vitest'; import type { PenNode } from '@zseven-w/pen-types'; -import { detectEdgeSectionPadding } from '../diagnostics/detectors-spacing'; +import { + detectEdgeSectionPadding, + detectStackedHorizontalPadding, +} from '../diagnostics/detectors-spacing'; // 2026-05-09 user report — image 6: "Categories" mobile section had its // chip text glued to the screen edge because both the page root and the @@ -241,3 +244,85 @@ describe('detectEdgeSectionPadding', () => { expect(issues[0].nodeId).toBe('page'); }); }); + +// 2026-05-10 user report — DeepSeek "Bistro" mobile food app shipped with +// root padding [0,16,0,16] AND a "Today's Specials" section padding [0,24], +// effective 40px gutter on a 375px page. Looked pinched / "too much +// padding". detectStackedHorizontalPadding catches the page-vs-section +// double-pad pattern (info-only — section can legitimately want larger +// inset for emphasis, so detect-only and let user/agent decide). + +describe('detectStackedHorizontalPadding', () => { + it('flags section whose horizontal padding stacks with mobile root', () => { + const root = mobileRoot( + [section('specials', [text('t1')], { padding: [0, 24] }), section('news', [text('t2')])], + [0, 16, 0, 16], + ); + const issues = detectStackedHorizontalPadding(root); + expect(issues).toHaveLength(1); + expect(issues[0].nodeId).toBe('specials'); + expect(issues[0].category).toBe('stacked-horizontal-padding'); + expect(issues[0].severity).toBe('info'); + expect(issues[0].reason).toMatch(/stacks with root/); + }); + + it('flags multiple offending sections in one root', () => { + const root = mobileRoot( + [ + section('a', [text('t1')], { padding: [0, 24] }), + section('b', [text('t2')], { padding: 16 }), + section('c', [text('t3')]), + ], + [0, 16, 0, 16], + ); + const issues = detectStackedHorizontalPadding(root); + expect(issues).toHaveLength(2); + expect(new Set(issues.map((i) => i.nodeId))).toEqual(new Set(['a', 'b'])); + }); + + it('does NOT flag when only root has horizontal padding (the goal state)', () => { + const root = mobileRoot( + [section('a', [text('t1')]), section('b', [text('t2')])], + [0, 16, 0, 16], + ); + expect(detectStackedHorizontalPadding(root)).toHaveLength(0); + }); + + it('does NOT flag when root has 0 horizontal padding (sections own gutter is the only one)', () => { + const root = mobileRoot([ + section('a', [text('t1')], { padding: [0, 24] }), + section('b', [text('t2')], { padding: [0, 24] }), + ]); + expect(detectStackedHorizontalPadding(root)).toHaveLength(0); + }); + + it('does NOT flag full-bleed roles (top-nav / hero / banner) even when their padding stacks', () => { + const root = mobileRoot( + [ + section('topnav', [text('Title')], { role: 'top-nav', padding: [0, 24] }), + section('hero', [text('h')], { role: 'hero', padding: [0, 24] }), + ], + [0, 16, 0, 16], + ); + expect(detectStackedHorizontalPadding(root)).toHaveLength(0); + }); + + it('does NOT flag desktop-shaped roots (component-internal padding stacking is legitimate)', () => { + // The detector is page-shape-only: a 1200px desktop card with a chip + // having internal padding is a totally normal nested-component pattern, + // not the page-gutter doubling we care about. + const root = { + id: 'page', + type: 'frame', + width: 1200, + height: 800, + layout: 'vertical', + padding: [0, 24, 0, 24], + children: [ + section('a', [text('t1')], { padding: [0, 16] }), + section('b', [text('t2')], { padding: [0, 16] }), + ], + } as unknown as PenNode; + expect(detectStackedHorizontalPadding(root)).toHaveLength(0); + }); +}); diff --git a/packages/pen-ai-skills/src/diagnostics/detectors-spacing.ts b/packages/pen-ai-skills/src/diagnostics/detectors-spacing.ts index 148fe80bc..6433184e1 100644 --- a/packages/pen-ai-skills/src/diagnostics/detectors-spacing.ts +++ b/packages/pen-ai-skills/src/diagnostics/detectors-spacing.ts @@ -31,6 +31,17 @@ function getPaddingLeft(node: PenNode): number { return 0; } +function getPaddingRight(node: PenNode): number { + const p = (node as unknown as { padding?: unknown }).padding; + if (typeof p === 'number') return p; + if (Array.isArray(p)) { + if (p.length === 4) return Number(p[1] ?? 0); + if (p.length === 2) return Number(p[1] ?? 0); + if (p.length === 1) return Number(p[0] ?? 0); + } + return 0; +} + function hasTextOrIconDescendant(node: PenNode): boolean { if (node.type === 'text') return true; if ((node.type as string) === 'icon_font') return true; @@ -162,3 +173,80 @@ export function detectEdgeSectionPadding(root: PenNode): Issue[] { } } } + +/** + * Aesthetic detector: page root and a direct content section both apply + * horizontal padding, producing a doubled inset. + * + * 2026-05-10 user-reported "Bistro" mobile design hit this: root carried + * `padding: [0, 16, 0, 16]` and `Today's Specials` section carried + * `padding: [0, 24]`. Effective gutter = 16 + 24 = 40px on a 375px page, + * leaving only 295px for content. Read as "too much padding" / pinched. + * + * The skill prompt now teaches "page gutter goes on ONE layer, not both" + * (see layout.md AESTHETIC HYGIENE). This detector is the cheap + * post-pass guard for when the LLM ignores the rule. + * + * Severity is INFO (detect-only). Auto-fix is tempting (zero out section + * horizontal padding, keep root) but a section may legitimately want a + * larger inset for visual emphasis (a hero card with deeper gutter than + * surrounding sections). Surface in the audit panel; let the user/agent + * decide. The skill rule covers the preventive side. + * + * Page-shape filter mirrors detectEdgeSectionPadding to avoid firing on + * components-with-internal-padding (chip → status-badge stacking is + * legitimate; the doubled-gutter problem is unique to mobile pages). + */ +export function detectStackedHorizontalPadding(root: PenNode): Issue[] { + const issues: Issue[] = []; + walk(root); + return issues; + + function walk(node: PenNode): void { + const width = (node as unknown as { width?: unknown }).width; + const height = (node as unknown as { height?: unknown }).height; + const looksLikeMobilePage = + node.type === 'frame' && + typeof width === 'number' && + width >= 320 && + width <= 480 && + typeof height === 'number' && + height >= 568 && + height >= width * 1.5; + + if ( + looksLikeMobilePage && + 'children' in node && + Array.isArray(node.children) && + node.children.length >= 2 + ) { + const rootL = getPaddingLeft(node); + const rootR = getPaddingRight(node); + if (rootL > 0 || rootR > 0) { + for (const child of (node as { children: PenNode[] }).children) { + if (child.type !== 'frame') continue; + const role = ((child as { role?: string }).role ?? '').toLowerCase(); + if (FULL_BLEED_ROLES.has(role)) continue; + const childL = getPaddingLeft(child); + const childR = getPaddingRight(child); + if (childL === 0 && childR === 0) continue; + // Section's horizontal padding stacks with root's. Flag the section + // (the offender — root is the established gutter holder). + issues.push({ + nodeId: child.id, + category: 'stacked-horizontal-padding', + severity: 'info', + property: 'padding', + currentValue: (child as unknown as { padding?: unknown }).padding, + suggestedValue: null, + reason: `section h-padding [${childL}/${childR}] stacks with root h-padding [${rootL}/${rootR}] — combined gutter ${rootL + childL}/${rootR + childR}`, + }); + } + } + } + + if ('children' in node && Array.isArray(node.children)) { + for (const c of node.children) walk(c); + } + } +} diff --git a/packages/pen-ai-skills/src/diagnostics/detectors.ts b/packages/pen-ai-skills/src/diagnostics/detectors.ts index 2bfcc229f..c0a009941 100644 --- a/packages/pen-ai-skills/src/diagnostics/detectors.ts +++ b/packages/pen-ai-skills/src/diagnostics/detectors.ts @@ -1,6 +1,6 @@ import type { PenNode, PenDocument } from '@zseven-w/pen-types'; import type { Issue } from './types'; -import { detectEdgeSectionPadding } from './detectors-spacing'; +import { detectEdgeSectionPadding, detectStackedHorizontalPadding } from './detectors-spacing'; import { detectTextBgContrast } from './detectors-typography'; import { colorContrast, parseHexColor, relativeLuminance } from './color-utils'; @@ -691,7 +691,7 @@ export function detectExcessiveFrameEffects(root: PenNode): Issue[] { } /** - * Run all 13 detectors and return the deduplicated combined issue list. + * Run all 14 detectors and return the deduplicated combined issue list. * Dedup key: `${nodeId}:${property}` (matches runPreValidationFixes). * On collision, the first issue wins (detector execution order below). */ @@ -709,6 +709,7 @@ export function detectAllIssues(root: PenNode, doc: PenDocument): Issue[] { ...detectMixedSiblingPadding(root), ...detectExcessiveFrameEffects(root), ...detectEdgeSectionPadding(root), + ...detectStackedHorizontalPadding(root), ...detectTextBgContrast(root, doc), ]; const seen = new Set(); diff --git a/packages/pen-ai-skills/src/diagnostics/index.ts b/packages/pen-ai-skills/src/diagnostics/index.ts index 60561089d..342fa32e8 100644 --- a/packages/pen-ai-skills/src/diagnostics/index.ts +++ b/packages/pen-ai-skills/src/diagnostics/index.ts @@ -14,6 +14,6 @@ export { detectExcessiveFrameEffects, detectAllIssues, } from './detectors'; -export { detectEdgeSectionPadding } from './detectors-spacing'; +export { detectEdgeSectionPadding, detectStackedHorizontalPadding } from './detectors-spacing'; export { detectTextBgContrast } from './detectors-typography'; export { colorContrast, parseHexColor, relativeLuminance } from './color-utils'; diff --git a/packages/pen-ai-skills/src/diagnostics/types.ts b/packages/pen-ai-skills/src/diagnostics/types.ts index 7e4d87a61..475174c3b 100644 --- a/packages/pen-ai-skills/src/diagnostics/types.ts +++ b/packages/pen-ai-skills/src/diagnostics/types.ts @@ -13,7 +13,8 @@ export type IssueCategory = | 'mixed-sibling-padding' | 'excessive-frame-effects' | 'edge-section-padding' - | 'text-bg-contrast'; + | 'text-bg-contrast' + | 'stacked-horizontal-padding'; export interface Issue { /** Node id where the issue was detected */ diff --git a/packages/pen-mcp/src/routes/debug-routes.ts b/packages/pen-mcp/src/routes/debug-routes.ts index 0ceee7ee7..fdf03a466 100644 --- a/packages/pen-mcp/src/routes/debug-routes.ts +++ b/packages/pen-mcp/src/routes/debug-routes.ts @@ -48,6 +48,7 @@ export const DEBUG_TOOL_DEFINITIONS = [ 'excessive-frame-effects', 'edge-section-padding', 'text-bg-contrast', + 'stacked-horizontal-padding', ], }, description: 'Filter to specific detector categories.', diff --git a/scripts/ab-corpus/check-stacked-padding.ts b/scripts/ab-corpus/check-stacked-padding.ts new file mode 100644 index 000000000..65015e647 --- /dev/null +++ b/scripts/ab-corpus/check-stacked-padding.ts @@ -0,0 +1,110 @@ +/** + * Empirical check: how common is "root has horizontal padding AND a + * direct child also has horizontal padding" in real LLM output? + * + * Drives the cost-benefit on adding a `detect-stacked-horizontal- + * padding` detector. The 2026-05-10 user-reported "Bistro" Mobile + * design hit this pattern — root [0,16,0,16] + section [0,24] = 40px + * effective gutter, which read as "too much padding". Need to know + * if it's a one-off or a recurring AI-output shape before adding a + * detector. + */ +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { parseModelOutput } from '@zseven-w/pen-ai-skills'; +import { applyToFreshDoc } from './apply'; + +interface JsonlRow { + promptId: string; + category: string; + difficulty: string; + variant: string; + rawOutput: string; +} + +function getHorizontalPadding(p: unknown): { left: number; right: number } { + if (typeof p === 'number') return { left: p, right: p }; + if (Array.isArray(p)) { + if (p.length === 4) return { left: Number(p[3] ?? 0), right: Number(p[1] ?? 0) }; + if (p.length === 2) return { left: Number(p[1] ?? 0), right: Number(p[1] ?? 0) }; + if (p.length === 1) return { left: Number(p[0] ?? 0), right: Number(p[0] ?? 0) }; + } + return { left: 0, right: 0 }; +} + +const FULL_BLEED_ROLES = new Set([ + 'hero', + 'banner', + 'cover', + 'header', + 'top-nav', + 'bottom-nav', + 'status-bar', + 'tab-bar', + 'tabbar', + 'navbar', +]); + +async function main(): Promise { + const runId = process.argv[2]; + if (!runId) { + console.error('usage: bun run scripts/ab-corpus/check-stacked-padding.ts '); + process.exit(1); + } + const path = join(import.meta.dir, 'runs', runId, 'scores.jsonl'); + const rows: JsonlRow[] = readFileSync(path, 'utf-8') + .split('\n') + .filter(Boolean) + .map((l) => JSON.parse(l)); + + let applied = 0; + let stacked = 0; + const examples: string[] = []; + + for (const r of rows) { + const parsed = parseModelOutput(r.rawOutput); + if (parsed.kind === 'garbage') continue; + const result = await applyToFreshDoc(parsed); + if (!result.ok || !result.doc) continue; + applied++; + const root = result.doc.children?.[0] as + | (Record & { padding?: unknown; children?: unknown[] }) + | undefined; + if (!root) continue; + const rootP = getHorizontalPadding(root.padding); + if (rootP.left === 0 && rootP.right === 0) continue; + if (!Array.isArray(root.children)) continue; + const offending: string[] = []; + for (const child of root.children as Array< + Record & { id?: string; role?: string; padding?: unknown } + >) { + if (child?.type !== 'frame') continue; + const role = String(child.role ?? '').toLowerCase(); + if (FULL_BLEED_ROLES.has(role)) continue; + const p = getHorizontalPadding(child.padding); + if (p.left > 0 || p.right > 0) offending.push(String(child.id ?? '?')); + } + if (offending.length > 0) { + stacked++; + if (examples.length < 8) { + examples.push( + ` ${r.promptId}/${r.variant} root=${rootP.left}+${rootP.right} offenders=${offending.length} (${offending.slice(0, 3).join(',')})`, + ); + } + } + } + + console.log(`applied: ${applied}`); + console.log( + `stacked horizontal padding (root H>0 AND ≥1 child section H>0): ${stacked} (${((stacked / applied) * 100).toFixed(1)}%)`, + ); + if (examples.length > 0) { + console.log('\nexamples:'); + for (const e of examples) console.log(e); + } +} + +main().catch((e) => { + console.error(e); + process.exit(1); +});