fix(pen-core): overlay converter only matches explicit layout=vertical
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.
This commit is contained in:
parent
999cbaa9c4
commit
c929c6a8b4
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
Loading…
Reference in a new issue