From a95be2860f0d2ee6e20d2a7ed81260e07cb34cab Mon Sep 17 00:00:00 2001 From: metaphorics <152830360+metaphorics@users.noreply.github.com> Date: Sun, 14 Jun 2026 08:40:12 +0900 Subject: [PATCH] fix(tui): honest editor-height floor and dead-branch cleanup Addresses Copilot + Codex review on #2521: - computeEditorMaxHeight returned 1 on terminals too small to host both the editor and the chrome reserve, but the bordered editor never renders fewer than 3 rows (2 border + 1 content). The cap now floors at that real minimum (EDITOR_MIN_RENDERED_ROWS) so it no longer misreports the rows the editor occupies; rendering is unchanged, and the contract is documented honestly (reserve holds once terminalRows >= 7). - #resolveOverlayLayout now always resolves maxHeight (?? availHeight), so the maxHeight !== undefined branch in effectiveHeight, the composite slice guard, and the number | undefined return type were dead. Tightened all three. - Mirrored the maxHeight-default contract in the render stress oracle (resolveExpectedOverlayLayout + compositeExpectedOverlays) so the randomized sweep validates the real clipped geometry instead of the obsolete unclipped one; updated the oracle helper test expectation accordingly. Editor-height tests rewritten to assert the real contract (reserve when the terminal can host both; pinned to the bordered minimum below that). --- packages/coding-agent/CHANGELOG.md | 2 +- .../src/modes/interactive-mode.ts | 19 +++++++++++++++---- .../test/editor-max-height.test.ts | 18 +++++++++++++++--- packages/tui/src/tui.ts | 9 +++++---- packages/tui/test/render-stress-harness.ts | 12 +++++------- .../tui/test/render-stress-oracles.test.ts | 2 +- 6 files changed, 42 insertions(+), 20 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index a202b0454..875f46710 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -4,7 +4,7 @@ ### Fixed -- Fixed the editor input box claiming a disproportionate share of small terminals (<=18 rows): the editor max-height floor (6 rows) ignored the available space. The height now yields to terminal size (`EDITOR_MIN_CHROME_ROWS`), always reserving rows for the transcript and status line. +- Fixed the editor input box claiming a disproportionate share of small terminals (<=18 rows): the editor max-height floor (6 rows) ignored the available space. The height now yields to terminal size (`EDITOR_MIN_CHROME_ROWS`), reserving rows for the transcript and status line whenever the terminal can host both; on terminals too small for both, the editor collapses to its real bordered minimum instead of overshooting a fictitious cap. ## [15.12.5] - 2026-06-13 ### Changed diff --git a/packages/coding-agent/src/modes/interactive-mode.ts b/packages/coding-agent/src/modes/interactive-mode.ts index d7b9a1904..ee491e72b 100644 --- a/packages/coding-agent/src/modes/interactive-mode.ts +++ b/packages/coding-agent/src/modes/interactive-mode.ts @@ -210,13 +210,24 @@ const EDITOR_MAX_HEIGHT_MIN = 6; const EDITOR_MAX_HEIGHT_MAX = 18; const EDITOR_RESERVED_ROWS = 12; const EDITOR_FALLBACK_ROWS = 24; -const EDITOR_MIN_CHROME_ROWS = 4; // rows reserved for transcript + status + chrome on tiny terms +const EDITOR_MIN_CHROME_ROWS = 4; // rows reserved for transcript + status on small terms +const EDITOR_MIN_RENDERED_ROWS = 3; // bordered editor floor: top+bottom border + 1 content row +/** + * Editor max-height cap for a terminal of `terminalRows` rows. + * + * Roomy terminals get the comfortable [6, 18] band. Small terminals shrink the + * cap so the editor leaves at least EDITOR_MIN_CHROME_ROWS rows for the + * transcript + status line. The editor is bordered, so it never renders fewer + * than EDITOR_MIN_RENDERED_ROWS rows; once the terminal is too small for both + * (terminalRows < EDITOR_MIN_RENDERED_ROWS + EDITOR_MIN_CHROME_ROWS) the cap is + * pinned to that floor — returning a smaller number would not shrink the editor + * any further, it would only misreport the rows it actually occupies. + */ export function computeEditorMaxHeight(terminalRows: number): number { const rows = Number.isFinite(terminalRows) && terminalRows > 0 ? terminalRows : EDITOR_FALLBACK_ROWS; - const desired = Math.max(EDITOR_MAX_HEIGHT_MIN, Math.min(EDITOR_MAX_HEIGHT_MAX, rows - EDITOR_RESERVED_ROWS)); - // Never let the editor crowd out the rest of the UI on small terminals. - return Math.max(1, Math.min(desired, rows - EDITOR_MIN_CHROME_ROWS)); + const comfortable = Math.max(EDITOR_MAX_HEIGHT_MIN, Math.min(EDITOR_MAX_HEIGHT_MAX, rows - EDITOR_RESERVED_ROWS)); + return Math.max(EDITOR_MIN_RENDERED_ROWS, Math.min(comfortable, rows - EDITOR_MIN_CHROME_ROWS)); } const HUD_NOTE_SUP_DIGITS: Record = { diff --git a/packages/coding-agent/test/editor-max-height.test.ts b/packages/coding-agent/test/editor-max-height.test.ts index 0cee1945c..67d7f405a 100644 --- a/packages/coding-agent/test/editor-max-height.test.ts +++ b/packages/coding-agent/test/editor-max-height.test.ts @@ -2,16 +2,28 @@ import { describe, expect, it } from "bun:test"; import { computeEditorMaxHeight } from "@oh-my-pi/pi-coding-agent/modes/interactive-mode"; describe("computeEditorMaxHeight", () => { - it("caps the editor while preserving chrome rows on small terminals", () => { + it("caps the editor within the comfortable band on roomy terminals", () => { expect(computeEditorMaxHeight(30)).toBe(18); expect(computeEditorMaxHeight(18)).toBe(6); expect(computeEditorMaxHeight(8)).toBe(4); - expect(computeEditorMaxHeight(5)).toBe(1); expect(computeEditorMaxHeight(Number.NaN)).toBe(12); expect(computeEditorMaxHeight(0)).toBe(12); + }); - for (let rows = 5; rows <= 18; rows += 1) { + it("reserves at least four chrome rows once the terminal can host both", () => { + // Editor floor (3 rendered rows, bordered) + chrome reserve (4) = 7 rows. + for (let rows = 7; rows <= 18; rows += 1) { expect(rows - computeEditorMaxHeight(rows)).toBeGreaterThanOrEqual(4); } }); + + it("pins the cap to the bordered editor's real minimum on tinier terminals", () => { + // Below 7 rows there is no room for both; the cap collapses to the editor's + // real rendered floor (2 border + 1 content) rather than a fictitious value + // the editor would silently overshoot. + expect(computeEditorMaxHeight(6)).toBe(3); + expect(computeEditorMaxHeight(5)).toBe(3); + expect(computeEditorMaxHeight(4)).toBe(3); + expect(computeEditorMaxHeight(1)).toBe(3); + }); }); diff --git a/packages/tui/src/tui.ts b/packages/tui/src/tui.ts index 4896989f3..e0fe66f2c 100644 --- a/packages/tui/src/tui.ts +++ b/packages/tui/src/tui.ts @@ -1875,7 +1875,7 @@ export class TUI extends Container { overlayHeight: number, termWidth: number, termHeight: number, - ): { width: number; row: number; col: number; maxHeight: number | undefined } { + ): { width: number; row: number; col: number; maxHeight: number } { const opt = options ?? {}; // Parse margin (clamp to non-negative) @@ -1905,8 +1905,9 @@ export class TUI extends Container { let maxHeight = parseSizeValue(opt.maxHeight, termHeight) ?? availHeight; maxHeight = Math.max(1, Math.min(maxHeight, availHeight)); - // Effective overlay height (may be clamped by maxHeight) - const effectiveHeight = maxHeight !== undefined ? Math.min(overlayHeight, maxHeight) : overlayHeight; + // Effective overlay height: maxHeight is always resolved (defaults to + // availHeight above), so the overlay is unconditionally clamped to fit. + const effectiveHeight = Math.min(overlayHeight, maxHeight); // === Resolve position === let row: number; @@ -2017,7 +2018,7 @@ export class TUI extends Container { // (width and maxHeight don't depend on overlay height). const { width, maxHeight } = this.#resolveOverlayLayout(options, 0, termWidth, termHeight); let overlayLines = component.render(width); - if (maxHeight !== undefined && overlayLines.length > maxHeight) { + if (overlayLines.length > maxHeight) { overlayLines = overlayLines.slice(0, maxHeight); } const { row, col } = this.#resolveOverlayLayout(options, overlayLines.length, termWidth, termHeight); diff --git a/packages/tui/test/render-stress-harness.ts b/packages/tui/test/render-stress-harness.ts index 7c01a212a..3c2277bbc 100644 --- a/packages/tui/test/render-stress-harness.ts +++ b/packages/tui/test/render-stress-harness.ts @@ -3004,7 +3004,7 @@ function compositeExpectedOverlays( if (!isExpectedOverlayVisible(entry, termWidth, termHeight)) continue; const firstLayout = resolveExpectedOverlayLayout(entry.options, 0, termWidth, termHeight); let overlayLines = entry.component.render(firstLayout.width); - if (firstLayout.maxHeight !== undefined && overlayLines.length > firstLayout.maxHeight) { + if (overlayLines.length > firstLayout.maxHeight) { overlayLines = overlayLines.slice(0, firstLayout.maxHeight); } const layout = resolveExpectedOverlayLayout(entry.options, overlayLines.length, termWidth, termHeight); @@ -3039,7 +3039,7 @@ export function resolveExpectedOverlayLayout( overlayHeight: number, termWidth: number, termHeight: number, -): { width: number; row: number; col: number; maxHeight: number | undefined } { +): { width: number; row: number; col: number; maxHeight: number } { const opt = options ?? {}; const margin = typeof opt.margin === "number" @@ -3056,11 +3056,9 @@ export function resolveExpectedOverlayLayout( width = Math.max(width, opt.minWidth); } width = Math.max(1, Math.min(width, availWidth)); - let maxHeight = parseOverlaySizeValue(opt.maxHeight, termHeight); - if (maxHeight !== undefined) { - maxHeight = Math.max(1, Math.min(maxHeight, availHeight)); - } - const effectiveHeight = maxHeight !== undefined ? Math.min(overlayHeight, maxHeight) : overlayHeight; + let maxHeight = parseOverlaySizeValue(opt.maxHeight, termHeight) ?? availHeight; + maxHeight = Math.max(1, Math.min(maxHeight, availHeight)); + const effectiveHeight = Math.min(overlayHeight, maxHeight); let row: number; let col: number; if (opt.row !== undefined) { diff --git a/packages/tui/test/render-stress-oracles.test.ts b/packages/tui/test/render-stress-oracles.test.ts index d36aec905..74ecbe124 100644 --- a/packages/tui/test/render-stress-oracles.test.ts +++ b/packages/tui/test/render-stress-oracles.test.ts @@ -51,7 +51,7 @@ describe("render stress oracle helpers", () => { width: 10, row: 6, col: 29, - maxHeight: undefined, + maxHeight: 8, }); });