fix(runtime): draw box borders per-edge without the min-size guard (Ink parity, G05+G15) (#37)
* fix(runtime): draw box borders per-edge without the min-size guard (Ink parity, G05+G15) Removed blanket `w<2||h<2` return from drawBorder; replaced it with a `w<1||h<1` degenerate guard. Vertical sides now start at `offsetY = top ? 1 : 0` and run for `Math.max(0, h - topRows - bottomRows)`, matching Ink render-border.ts:133. Fixes: (G05) a 1-cell-tall box with only side rails rendered nothing; (G15) with borderTop=false the left/right rails were shifted one row down. Updated 4 existing snapshots that encoded the old buggy behavior and added 3 new tests that verified red before green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(parity): ledger — G05+G15 pr-open, reconcile G03 merged, log snapshot decision Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Yunfei He
@@ -24,6 +24,7 @@ Non-obvious calls made while fixing gaps, recorded for review in the final repor
|
||||
- _"Box backgroundColor survives border rendering"_: unchanged, still passes (the inner fill keeps the bg).
|
||||
- Added _"Box backgroundColor does not bleed onto border glyphs (Ink parity)"_ as the failing-first reproduction.
|
||||
- **2026-05-30 — G02 (useAnimation throttle):** to actually fix the gap by default (not just when a caller passes maxFps), `maxFps` now defaults to **30** (Ink: `options.maxFps ?? 30`) and a single `renderThrottleMs = ceil(1000/maxFps)` feeds BOTH the commit scheduler and `createAnimationScheduler`, mirroring Ink's one-value architecture. Behavior change: the default commit-throttle shifts 32ms→34ms (no test depended on 32ms; explicit `maxFps` already drove commit cadence on main). Test rewrite: the pre-existing `"delta accounts for throttled ticks"` test was weak/self-contradictory (asserted only `delta>0`, passed trivially against the bug) — replaced with `"delta accumulates across coalesced ticks…"` (maxFps:5, 200ms window) plus a default-path test using a count-based assertion (≈31 rendered ticks unfixed vs <15 fixed) so it deterministically discriminates. Both verified red on unfixed.
|
||||
- **2026-05-30 — G05+G15 (border geometry):** rewrote `paint.ts` drawBorder to draw each edge independently (mirroring Ink render-border.ts): removed the blanket `w<2||h<2` early-return (→ `w<1||h<1`) and changed the vertical-side loop to start at `offsetY = top?1:0` with run length `max(0, h - visibleTop - visibleBottom)`. Per conflict policy, 4 existing snapshot tests that encoded the buggy hide-top/bottom output were updated to Ink-correct values (rails on the content row, not shifted/missing) — codex traced each against Ink render-border.ts and confirmed they match (not just code-blessed). Normal (top+bottom) boxes are byte-identical to before.
|
||||
- **2026-05-29 — G06 REFUTED (false positive from the audit):** the audit claimed `<Transform>`'s fn gets a hardcoded index `0` "instead of the childNode index". Re-verification against Ink `output.ts:230-239` shows Ink's index is the **line index** (transformers apply per output line: `transformer(line, index)`), not a child index — the audit misread it. vue-tui **already** applies per-line line indices for multi-line (block) transforms via the yoga-carrier path: the existing tests `transform with multiple lines` → `[0: hello world]\n[1: goodbye world]` and transform-yoga `[0: hello]\n[1: world]` pass on unmodified code. `paint.ts:314`'s `transform(innerText, 0)` is only the inline `<Transform>`-inside-`<Text>` path, whose content is a single logical line where `0` matches Ink (all inline tests assert `[0: …]`). No observable gap; not fixed.
|
||||
|
||||
## Confirmed gaps
|
||||
@@ -34,9 +35,9 @@ Non-obvious calls made while fixing gaps, recorded for review in the final repor
|
||||
| --- | ------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------- | ------- | ---------------------------------- | --- |
|
||||
| G01 | static-newline-spacer | Static keeps every already-written item permanently mounted instead of unmounting it | P1 | merged | `fix/parity-static-unmount` | #34 |
|
||||
| G02 | app-exit-instances-animation-sr | useAnimation does not coalesce ticks within the render-throttle window — delta does not 'account for throttled renders' | P1 | merged | `fix/parity-useanimation-throttle` | #35 |
|
||||
| G03 | render-lifecycle-reconciler | Live screen-reader render path is missing; commit() always paints the visual grid | P1 | pr-open | `fix/parity-sr-render` | #36 |
|
||||
| G03 | render-lifecycle-reconciler | Live screen-reader render path is missing; commit() always paints the visual grid | P1 | merged | `fix/parity-sr-render` | #36 |
|
||||
| G04 | box-layout-border | Border edges incorrectly inherit the Box backgroundColor | P2 | merged | `fix/parity-border-bg` | #30 |
|
||||
| G05 | box-layout-border | Borders skipped when content area is 1 cell tall or wide (w<2 / h<2 guard) | P2 | todo | — | — |
|
||||
| G05 | box-layout-border | Borders skipped when content area is 1 cell tall or wide (w<2 / h<2 guard) | P2 | pr-open | `fix/parity-border-1cell` | #37 |
|
||||
| G06 | text-wrap-transform | Nested <Transform>/<Text> transform fn receives hardcoded index 0 instead of childNode index | P2 | refuted | — | — |
|
||||
| G07 | input-keypress-kitty-paste | Kitty-protocol Ctrl+C triggers app exit in vue-tui but only suppresses the handler in Ink | P2 | todo | — | — |
|
||||
| G08 | focus | useFocus does not react to changes in the id prop | P2 | merged | `fix/parity-usefocus-id` | #31 |
|
||||
@@ -46,7 +47,7 @@ Non-obvious calls made while fixing gaps, recorded for review in the final repor
|
||||
| G12 | render-lifecycle-reconciler | Renderer frame width/rows lack terminal-size fallback (only ?? defaults) | P2 | merged | `fix/parity-renderer-size` | #33 |
|
||||
| G13 | box-layout-border | Custom border style objects (BoxStyle) not supported | P3 | todo | — | — |
|
||||
| G14 | app-exit-instances-animation-sr | No per-stdout instance reuse/guard — two concurrent renderers can compete for the same stdout | P3 | todo | — | — |
|
||||
| G15 | box-layout-border | Vertical border sides not shifted up when borderTop=false (Ink offsetY) — left/right rails mispositioned | P2 | todo | — | — |
|
||||
| G15 | box-layout-border | Vertical border sides not shifted up when borderTop=false (Ink offsetY) — left/right rails mispositioned | P2 | pr-open | `fix/parity-border-1cell` | #37 |
|
||||
| G16 | box-layout-border | Per-edge borderDimColor=false cannot override general borderDimColor (`\|\| dimAll` vs Ink's `??`) | P3 | todo | — | — |
|
||||
| G17 | render-lifecycle-reconciler | Screen-reader live-path edges: <Static> still grid-painted (Ink linearizes, skipStaticElements:false) + empty SR frame gets a trailing newline (Ink writes wrapped output directly) | P3 | todo | — | — |
|
||||
|
||||
|
||||
@@ -676,8 +676,9 @@ test("render border edge changes after update when borderStyle is unchanged", as
|
||||
|
||||
showTop.value = false;
|
||||
await nextTick();
|
||||
// G15 fix: with borderTop=false, side rails now start at row 0 (Ink parity)
|
||||
expect(lastFrame()).toMatchInlineSnapshot(`
|
||||
" Content
|
||||
"│Content│
|
||||
╰───────╯"
|
||||
`);
|
||||
|
||||
@@ -704,9 +705,10 @@ test("hide top border", async ({ expect }) => {
|
||||
)),
|
||||
{ columns: 100 },
|
||||
);
|
||||
// G15 fix: side rails now start at row 0 when borderTop=false (Ink parity)
|
||||
expect(lastFrame()).toMatchInlineSnapshot(`
|
||||
"Above
|
||||
Content
|
||||
│Content│
|
||||
╰───────╯
|
||||
Below"
|
||||
`);
|
||||
@@ -726,10 +728,11 @@ test("hide bottom border", async ({ expect }) => {
|
||||
)),
|
||||
{ columns: 100 },
|
||||
);
|
||||
// G15 fix: side rails now span the full content height when borderBottom=false (Ink parity)
|
||||
expect(lastFrame()).toMatchInlineSnapshot(`
|
||||
"Above
|
||||
╭───────╮
|
||||
Content
|
||||
│Content│
|
||||
Below"
|
||||
`);
|
||||
});
|
||||
@@ -748,9 +751,11 @@ test("hide top and bottom borders", async ({ expect }) => {
|
||||
)),
|
||||
{ columns: 100 },
|
||||
);
|
||||
// G05+G15 fix: side rails now render even at h=1 when both top and bottom are
|
||||
// hidden — the per-edge geometry draws them at the single content row (Ink parity)
|
||||
expect(lastFrame()).toMatchInlineSnapshot(`
|
||||
"Above
|
||||
Content
|
||||
│Content│
|
||||
Below"
|
||||
`);
|
||||
});
|
||||
@@ -1199,6 +1204,71 @@ test("foreground, background and dim combine correctly", async ({ expect }) => {
|
||||
expect(frame).toContain("[2m");
|
||||
});
|
||||
|
||||
// G05 — height-1 box with side-only borders renders rails (Ink parity)
|
||||
// A box that is exactly 1 cell tall (borderTop/Bottom=false) must still render
|
||||
// the left and right rails on the single content row, producing │X│ output.
|
||||
// Previously the blanket `w < 2 || h < 2` guard aborted the entire drawBorder.
|
||||
test("G05: height-1 box with side-only borders renders rails", async ({ expect }) => {
|
||||
const { lastFrame } = await render(
|
||||
defineComponent(() => () => (
|
||||
// height=1 → only 1 row; no top/bottom borders, only left+right rails
|
||||
<Box borderStyle="single" borderTop={false} borderBottom={false} alignSelf="flex-start">
|
||||
<Text>X</Text>
|
||||
</Box>
|
||||
)),
|
||||
{ columns: 20 },
|
||||
);
|
||||
const frame = stripAnsi(lastFrame()!);
|
||||
// The single row must carry the left AND right rails
|
||||
expect(frame).toContain("│X│");
|
||||
});
|
||||
|
||||
// G05 — width-1 box with top/bottom-only borders renders the edge glyphs (Ink parity)
|
||||
// A box that is exactly 1 cell wide (after removing left/right borders) must still
|
||||
// render the top and bottom horizontal edges, each a single glyph.
|
||||
test("G05: width-1 box with top/bottom-only borders renders edge glyphs", async ({ expect }) => {
|
||||
const { lastFrame } = await render(
|
||||
defineComponent(() => () => (
|
||||
// width=1 content area; no left/right borders; only top+bottom
|
||||
<Box
|
||||
borderStyle="single"
|
||||
borderLeft={false}
|
||||
borderRight={false}
|
||||
width={1}
|
||||
alignSelf="flex-start"
|
||||
>
|
||||
<Text>X</Text>
|
||||
</Box>
|
||||
)),
|
||||
{ columns: 20 },
|
||||
);
|
||||
const frame = stripAnsi(lastFrame()!);
|
||||
const lines = frame.split("\n");
|
||||
// Top edge must be present (─)
|
||||
expect(lines[0]).toContain("─");
|
||||
// Bottom edge must be present (─)
|
||||
expect(lines[lines.length - 1]).toContain("─");
|
||||
});
|
||||
|
||||
// G15 — vertical side rails are not shifted down when borderTop=false (Ink parity)
|
||||
// With borderTop=false, the vertical sides must start at row 0 (the content row),
|
||||
// not row 1 (which is how the buggy i=1 loop positioned them).
|
||||
test("G15: side rails appear on the first content row when borderTop=false", async ({ expect }) => {
|
||||
const { lastFrame } = await render(
|
||||
defineComponent(() => () => (
|
||||
<Box borderStyle="single" borderTop={false} alignSelf="flex-start">
|
||||
<Text>Content</Text>
|
||||
</Box>
|
||||
)),
|
||||
{ columns: 20 },
|
||||
);
|
||||
const frame = stripAnsi(lastFrame()!);
|
||||
const lines = frame.split("\n");
|
||||
// First line is the content row (no top border); it must carry both rails.
|
||||
// The "single" style uses │ (U+2502 BOX DRAWINGS LIGHT VERTICAL), not ASCII |.
|
||||
expect(lines[0]).toMatch(/^│.*│$/);
|
||||
});
|
||||
|
||||
// borderDimColor should not dim styled child Text touching left edge
|
||||
test("borderDimColor does not dim styled child Text touching left edge", async ({ expect }) => {
|
||||
const { lastFrame } = await render(
|
||||
|
||||
@@ -335,7 +335,13 @@ function drawBorder(
|
||||
if (!style) return;
|
||||
const chars = (cliBoxes as unknown as Record<string, BoxStyle | undefined>)[style];
|
||||
if (!chars) return;
|
||||
if (w < 2 || h < 2) return;
|
||||
// No blanket min-size guard here — each edge is drawn independently when it is
|
||||
// visible and its run length is ≥ 1. This matches Ink's render-border.ts which
|
||||
// has no such guard and draws every edge on its own, so a 1-cell-tall box with
|
||||
// only side rails still renders │X│ (G05), and a 1-cell-wide box with only
|
||||
// top/bottom still renders the top/bottom glyph. Guard individual repeat()
|
||||
// counts with Math.max(0, …) so a degenerate dimension doesn't throw.
|
||||
if (w < 1 || h < 1) return;
|
||||
|
||||
const top = props["borderTop"] !== false;
|
||||
const bottom = props["borderBottom"] !== false;
|
||||
@@ -375,9 +381,17 @@ function drawBorder(
|
||||
const raw = bl + chars.bottom.repeat(fill) + br;
|
||||
output.write(x, y + h - 1, [colorizeEdge(safeSliceEnd(raw, w), "bottom")], transformers);
|
||||
}
|
||||
for (let i = 1; i < h - 1; i++) {
|
||||
if (left) output.write(x, y + i, [colorizeEdge(chars.left, "left")], transformers);
|
||||
if (right) output.write(x + w - 1, y + i, [colorizeEdge(chars.right, "right")], transformers);
|
||||
|
||||
// Ink parity (render-border.ts:133): vertical sides start at y + offsetY where
|
||||
// offsetY = showTopBorder ? 1 : 0. Without this, the loop starting at i=1
|
||||
// always skips row 0, shifting rails one row down when borderTop=false (G15).
|
||||
// The run length equals h minus the visible top/bottom rows, clamped ≥ 0.
|
||||
const offsetY = top ? 1 : 0;
|
||||
const verticalRun = Math.max(0, h - (top ? 1 : 0) - (bottom ? 1 : 0));
|
||||
for (let i = 0; i < verticalRun; i++) {
|
||||
if (left) output.write(x, y + offsetY + i, [colorizeEdge(chars.left, "left")], transformers);
|
||||
if (right)
|
||||
output.write(x + w - 1, y + offsetY + i, [colorizeEdge(chars.right, "right")], transformers);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user