From 117ce70c0c17de5ef19f18afcafa068f186c33e6 Mon Sep 17 00:00:00 2001 From: Mohamed Khaled Date: Wed, 19 Aug 2026 18:17:31 +0300 Subject: [PATCH] fix(tui): page multi-column pickers when the grid outgrows the terminal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit usePickerViewport only engaged for single-column pickers, assuming multi-column grids compress vertically enough. The 23-option framework picker renders a 12-row grid that, with the intro screen's chrome, needs 28 terminal rows — anything shorter overflowed the viewport and Ink's repaint fused adjacent lines (#1111). The viewport now measures the grid's real height (ceil(count / columns) rows) against the budget and falls back to the existing flat paged view when it doesn't fit. Navigation follows: while paging is engaged the picker is single-column, so the left/right column bindings switch off. CHROME_OVERHEAD moves 13 -> 15 to match the intro screen's measured chrome, and the budget floor drops 5 -> 3 so the page keeps shrinking with the terminal instead of pushing the frame past it. Verified in a scripted pty: the framework-select screen now renders clean from 19 rows up (previously corrupted below 28); the paging decision is pinned by unit tests. --- src/ui/tui/__tests__/picker-viewport.test.ts | 114 +++++++++++++++++++ src/ui/tui/primitives/PickerMenu.tsx | 88 ++++++++++---- 2 files changed, 178 insertions(+), 24 deletions(-) create mode 100644 src/ui/tui/__tests__/picker-viewport.test.ts diff --git a/src/ui/tui/__tests__/picker-viewport.test.ts b/src/ui/tui/__tests__/picker-viewport.test.ts new file mode 100644 index 000000000..50937c1b4 --- /dev/null +++ b/src/ui/tui/__tests__/picker-viewport.test.ts @@ -0,0 +1,114 @@ +import { computePickerViewport } from '@ui/tui/primitives/PickerMenu'; + +/** + * The framework picker's shape: 23 options, label-only rows, a filter row, + * two columns (#1111). Its 12-row grid plus the intro screen's chrome needs + * 28 terminal rows — anything shorter must page instead of overflowing. + */ +const FRAMEWORK_COUNT = 23; +const LABEL_ROW = 1; +const FILTER_CHROME = 1; + +describe('computePickerViewport', () => { + describe('two-column framework picker (#1111)', () => { + it('keeps the grid on a tall terminal', () => { + const vp = computePickerViewport( + FRAMEWORK_COUNT, + LABEL_ROW, + FILTER_CHROME, + 2, + 0, + 50, + ); + expect(vp.needsScroll).toBe(false); + expect(vp.start).toBe(0); + expect(vp.end).toBe(FRAMEWORK_COUNT); + }); + + it('keeps the grid at exactly 28 rows — the smallest healthy height', () => { + const vp = computePickerViewport( + FRAMEWORK_COUNT, + LABEL_ROW, + FILTER_CHROME, + 2, + 0, + 28, + ); + expect(vp.needsScroll).toBe(false); + }); + + it('pages at 27 rows — the first height that used to corrupt', () => { + const vp = computePickerViewport( + FRAMEWORK_COUNT, + LABEL_ROW, + FILTER_CHROME, + 2, + 0, + 27, + ); + expect(vp.needsScroll).toBe(true); + // budget 11, minus the two "N more" indicator rows + expect(vp.end - vp.start).toBe(9); + expect(vp.hiddenBelow).toBe(FRAMEWORK_COUNT - 9); + }); + + it('shrinks the page with the terminal instead of overflowing', () => { + const at20 = computePickerViewport( + FRAMEWORK_COUNT, + LABEL_ROW, + FILTER_CHROME, + 2, + 0, + 20, + ); + expect(at20.needsScroll).toBe(true); + expect(at20.end - at20.start).toBe(2); + + // The floor: two indicator rows plus one option, never zero. + const tiny = computePickerViewport( + FRAMEWORK_COUNT, + LABEL_ROW, + FILTER_CHROME, + 2, + 0, + 10, + ); + expect(tiny.needsScroll).toBe(true); + expect(tiny.end - tiny.start).toBe(1); + }); + }); + + it('leaves a grid alone when it genuinely fits the height', () => { + // 6 options in 2 columns is a 3-row grid — fine even at 20 rows. + const vp = computePickerViewport(6, LABEL_ROW, 0, 2, 0, 20); + expect(vp.needsScroll).toBe(false); + }); + + it('never pages lists shorter than the minimum count', () => { + const vp = computePickerViewport(4, LABEL_ROW, 0, 1, 0, 10); + expect(vp.needsScroll).toBe(false); + }); + + it('pages a long single-column list exactly as before', () => { + const vp = computePickerViewport(23, LABEL_ROW, FILTER_CHROME, 1, 0, 50); + expect(vp.needsScroll).toBe(true); + expect(vp.end - vp.start).toBe(10); // MAX_LIST_ROWS 12 minus indicators + }); + + it('derives the visible page from the focused index', () => { + const vp = computePickerViewport(23, LABEL_ROW, FILTER_CHROME, 2, 12, 27); + // perPage 9: focused 12 sits on the second page. + expect(vp.start).toBe(9); + expect(vp.end).toBe(18); + expect(vp.hiddenAbove).toBe(9); + expect(vp.hiddenBelow).toBe(5); + }); + + it('wraps page stepping in both directions', () => { + const vp = computePickerViewport(23, LABEL_ROW, FILTER_CHROME, 2, 0, 27); + // perPage 9 → pages start at 0, 9, 18. + expect(vp.pageStep(0, 1)).toBe(9); + expect(vp.pageStep(18, 1)).toBe(0); + expect(vp.pageStep(0, -1)).toBe(18); + }); +}); diff --git a/src/ui/tui/primitives/PickerMenu.tsx b/src/ui/tui/primitives/PickerMenu.tsx index 9ecd28223..0295f84c9 100644 --- a/src/ui/tui/primitives/PickerMenu.tsx +++ b/src/ui/tui/primitives/PickerMenu.tsx @@ -111,8 +111,14 @@ function lastEnabled( * deliberately generous estimate: overshooting just shows a few fewer rows, * whereas undershooting lets a long list overflow the viewport (the bug this * windowing guards against). Mirrors GroupedPickerMenu's budgeting. + * + * 15 is measured, not guessed: the intro screen (title bar, wizard title, + * two subtitle lines, detection notice, prompt, spacing, hints bar) consumes + * 15 rows around its picker plus the filter row counted via `chromeBelow` — + * the framework picker corrupted at 27 terminal rows under the previous + * value of 13 (#1111). */ -const CHROME_OVERHEAD = 13; +const CHROME_OVERHEAD = 15; /** * Max visual rows a picker renders regardless of terminal height. Without a * ceiling a tall terminal lets a long list fill the whole viewport, which @@ -127,7 +133,7 @@ const MIN_COUNT_TO_PAGE = 5; /** Width the multi-select wraps option descriptions to (matches the render). */ const DESCRIPTION_WIDTH = 56; -interface PickerViewport { +export interface PickerViewport { needsScroll: boolean; /** First option index on the current page. */ start: number; @@ -140,27 +146,37 @@ interface PickerViewport { } /** - * Pages a single-column option list to the terminal height. The visible page - * is derived from the focused index — no scroll state — so ↑/↓ flip pages as - * focus crosses a page edge and n/p jump a whole page. Pages hold a fixed - * option count sized to the tallest row (`rowCost`), trading a sparser page - * on mixed-height lists for arithmetic-only paging. Engages only for - * single-column pickers — multi-column grids already compress vertically. + * Pages an option list to the terminal height. The visible page is derived + * from the focused index — no scroll state — so ↑/↓ flip pages as focus + * crosses a page edge and n/p jump a whole page. Pages hold a fixed option + * count sized to the tallest row (`rowCost`), trading a sparser page on + * mixed-height lists for arithmetic-only paging. + * + * A multi-column grid compresses vertically to ceil(count / columns) rows, + * but that can still outgrow a short terminal — 23 options in 2 columns is + * a 12-row grid, taller than a 24-row viewport leaves (#1111). When the + * grid is too tall the picker pages, falling back to the flat single-column + * page the render already knows how to draw. + * + * Exported for unit tests; components use the `usePickerViewport` wrapper. */ -function usePickerViewport( +export function computePickerViewport( count: number, rowCost: number, chromeBelow: number, - enabled: boolean, + columns: number, focused: number, + termRows: number, ): PickerViewport { - const [, termRows] = useStdoutDimensions(); + // Floor of 3: two indicator rows plus at least one option. A one-option + // page on a very short terminal is cramped but correct — a taller floor + // would push the frame past the viewport and corrupt the paint instead. const budget = Math.max( - 5, + 3, Math.min(termRows - CHROME_OVERHEAD - chromeBelow, MAX_LIST_ROWS), ); - const needsScroll = - enabled && count >= MIN_COUNT_TO_PAGE && count * rowCost > budget; + const gridRows = Math.ceil(count / columns) * rowCost; + const needsScroll = count >= MIN_COUNT_TO_PAGE && gridRows > budget; // Reserve two rows for the "↑/↓ N more" indicators. const perPage = needsScroll ? Math.max(1, Math.floor((budget - 2) / rowCost)) @@ -179,6 +195,24 @@ function usePickerViewport( }; } +function usePickerViewport( + count: number, + rowCost: number, + chromeBelow: number, + columns: number, + focused: number, +): PickerViewport { + const [, termRows] = useStdoutDimensions(); + return computePickerViewport( + count, + rowCost, + chromeBelow, + columns, + focused, + termRows, + ); +} + interface PickerMenuProps { message?: string; options: PickerOption[]; @@ -327,15 +361,18 @@ const SinglePickerMenu = ({ onSelect: (value: T | T[]) => void; }) => { const [focused, setFocused] = useState(() => firstEnabled(options)); - const rows = Math.ceil(options.length / columns); // Single-select rows are label-only (no descriptions): one line plus margin. const viewport = usePickerViewport( options.length, 1 + optionMarginBottom, filter !== null ? 1 : 0, - columns === 1, + columns, focused, ); + // A paged list draws one flat column, so navigation has to match what's on + // screen: treat the grid as single-column while paging is engaged. + const effectiveColumns = viewport.needsScroll ? 1 : columns; + const rows = Math.ceil(options.length / effectiveColumns); // Re-validate focus when the options change while mounted \u2014 a list // that shrinks or disables entries can leave `focused` pointing at a @@ -391,7 +428,7 @@ const SinglePickerMenu = ({ }, ]; - if (columns > 1) { + if (effectiveColumns > 1) { bindings.splice(1, 0, { match: [KeyMatch.LeftArrow, KeyMatch.RightArrow], label: '\u2190\u2192', @@ -402,11 +439,11 @@ const SinglePickerMenu = ({ let next = focused; if (key.leftArrow) { - const prevCol = col > 0 ? col - 1 : columns - 1; + const prevCol = col > 0 ? col - 1 : effectiveColumns - 1; next = Math.min(prevCol * rows + row, options.length - 1); } if (key.rightArrow) { - const nextCol = col < columns - 1 ? col + 1 : 0; + const nextCol = col < effectiveColumns - 1 ? col + 1 : 0; next = Math.min(nextCol * rows + row, options.length - 1); } // Landing on a disabled option slides to the column's nearest @@ -553,7 +590,6 @@ const MultiPickerMenu = ({ const [focused, setFocused] = useState(() => firstEnabled(options)); // When true, the cursor is on the Confirm button rather than an option. const [onButton, setOnButton] = useState(false); - const rows = Math.ceil(options.length / columns); // A row is its label line plus any margin; a description adds one line per // wrapped line beneath the label. Pages size to the tallest row. const rowCost = options.reduce( @@ -572,9 +608,13 @@ const MultiPickerMenu = ({ options.length, rowCost, CONFIRM_CHROME + (filter !== null ? 1 : 0), - columns === 1, + columns, focused, ); + // A paged list draws one flat column, so navigation has to match what's on + // screen: treat the grid as single-column while paging is engaged. + const effectiveColumns = viewport.needsScroll ? 1 : columns; + const rows = Math.ceil(options.length / effectiveColumns); // Re-validate focus when the options change while mounted — a list // that shrinks or disables entries can leave `focused` pointing at a @@ -689,7 +729,7 @@ const MultiPickerMenu = ({ }, ]; - if (columns > 1) { + if (effectiveColumns > 1) { bindings.splice(1, 0, { match: [KeyMatch.LeftArrow, KeyMatch.RightArrow], label: '\u2190\u2192', @@ -701,11 +741,11 @@ const MultiPickerMenu = ({ let next = focused; if (key.leftArrow) { - const prevCol = col > 0 ? col - 1 : columns - 1; + const prevCol = col > 0 ? col - 1 : effectiveColumns - 1; next = Math.min(prevCol * rows + row, options.length - 1); } if (key.rightArrow) { - const nextCol = col < columns - 1 ? col + 1 : 0; + const nextCol = col < effectiveColumns - 1 ? col + 1 : 0; next = Math.min(nextCol * rows + row, options.length - 1); } // Landing on a disabled option slides to the column's nearest