From c929c6a8b413e7eebb76d869f05ebba6955606de Mon Sep 17 00:00:00 2001 From: Fini Date: Tue, 5 May 2026 12:23:32 +0800 Subject: [PATCH] fix(pen-core): overlay converter only matches explicit layout=vertical MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex flagged: when the convert pass runs BEFORE normalizeTreeLayout (required to preserve child x/y offsets — see 2fa66bc1), accepting \`layout === undefined\` as a vertical signal mis-classifies layout-less horizontal rows. A model that emits two equal-height images side by side without an explicit \`layout\` field intends a horizontal row; \`inferLayout\` (which normalize later runs) often agrees. The earlier converter saw the absent keyword as "vertical-shaped" and flipped the row to absolute, collapsing both images to (0,0). Tightened the gate to require explicit \`layout: 'vertical'\`. A hero that omits the keyword is now an acceptable miss — the convert pass leaves it for normalize to classify, after which nothing else fires the layered-detection rule (normalize would have stripped the children's x/y by then anyway, so even running convert again post-normalize wouldn't help). The cost is a small miss rate on extremely sloppy hero outputs; the benefit is no false positives on legit horizontal rows. New regression test: layout-less frame with two side-by-side height-200 images stays untouched. Verified by reverting the gate to also accept \`undefined\` — the new test correctly fails ("expected false to be true"). All 8 tests pass with the tightened gate. --- ...onvert-stacked-overlay-to-absolute.test.ts | 33 +++++++++++++++++++ .../convert-stacked-overlay-to-absolute.ts | 16 +++++++-- 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/packages/pen-core/src/__tests__/convert-stacked-overlay-to-absolute.test.ts b/packages/pen-core/src/__tests__/convert-stacked-overlay-to-absolute.test.ts index cbb9362bd..09a3a86c5 100644 --- a/packages/pen-core/src/__tests__/convert-stacked-overlay-to-absolute.test.ts +++ b/packages/pen-core/src/__tests__/convert-stacked-overlay-to-absolute.test.ts @@ -123,6 +123,39 @@ describe('convertStackedOverlayToAbsolute', () => { expect((card as PenNode & { layout?: string }).layout).toBe('vertical'); }); + it("doesn't flip layout-less horizontal rows (Codex regression on pre-normalize timing)", () => { + // Codex flag: when the convert pass runs BEFORE normalizeTreeLayout + // (which is required to preserve child x/y offsets — see the + // "preserves child offsets" test below), a layout-less frame + // hasn't been classified yet. Earlier I accepted + // `layout === undefined` thinking inferLayout would say vertical, + // but inferLayout reads the children's shape and a row of side- + // by-side images / rectangles often resolves to horizontal. + // Treating layout-less containers as vertical-by-default would + // mis-flip those rows to absolute and the side-by-side images + // would collapse to overlapping at (0,0). + // + // Fix: only accept explicit `layout: 'vertical'`. Layout-less + // frames are left for normalize to classify; if a hero + // genuinely omits the keyword, that's an acceptable miss. + const layoutlessRow = frame({ + id: 'maybe-row', + width: 'fill_container', + height: 200, + // NO layout field — model emitted a row of two equal-height + // images side by side and let inference figure it out. + children: [ + image({ id: 'left', width: 'fill_container', height: 200 }), + image({ id: 'right', width: 'fill_container', height: 200 }), + ], + }); + const root = frame({ id: 'root', children: [layoutlessRow] }); + const changed = convertStackedOverlayToAbsolute(root); + expect(changed).toBe(false); + // Layout untouched — we leave classification to normalizeTreeLayout. + expect((layoutlessRow as PenNode & { layout?: string }).layout).toBeUndefined(); + }); + it('respects an explicit layout=horizontal (not the bug shape)', () => { // A horizontal-layout container with side-by-side image+overlay // is NOT the layered-hero pattern — the model likely meant a diff --git a/packages/pen-core/src/layout/convert-stacked-overlay-to-absolute.ts b/packages/pen-core/src/layout/convert-stacked-overlay-to-absolute.ts index afb32fe36..835dbe933 100644 --- a/packages/pen-core/src/layout/convert-stacked-overlay-to-absolute.ts +++ b/packages/pen-core/src/layout/convert-stacked-overlay-to-absolute.ts @@ -25,8 +25,18 @@ import type { PenNode } from '@zseven-w/pen-types'; * * Pattern detection (conservative — false positives are worse than * misses): - * - The frame has `layout: 'vertical'` (or undefined, which - * `inferLayout` would also resolve to vertical). + * - The frame has `layout: 'vertical'` EXPLICITLY. We do NOT + * accept `undefined` here even though `inferLayout` would + * usually resolve it to vertical: a layout-less frame may also + * be the model's implicit way of saying "lay these out + * horizontally" (a row of icons with no x/y), and we don't + * want to flip those to absolute. Pre-`normalizeTreeLayout` + * timing makes this critical — by the time normalize fills + * in the inferred layout, this pass has already run, so we + * can't rely on a normalized field. If `layout` isn't there, + * we let normalize run first; the rare hero that omits the + * explicit `layout: 'vertical'` keyword is an acceptable + * miss. * - The frame has a NUMERIC fixed height `H`. * - At least 2 of the children are visually-large background- * candidate types (`image`, `rectangle`, or `frame`) AND have @@ -51,7 +61,7 @@ export function convertStackedOverlayToAbsolute(rootFrame: PenNode): boolean { height?: unknown; children?: PenNode[]; }; - if ((c.layout === 'vertical' || c.layout === undefined) && typeof c.height === 'number') { + if (c.layout === 'vertical' && typeof c.height === 'number') { const containerH = c.height; const children = Array.isArray(c.children) ? c.children : []; let bgLike = 0;