From 29d6ff7e84c84e95b056304f96154a00017027d8 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 07:48:23 +0000 Subject: [PATCH 1/4] Bound List's height in the modal overlay so long pickers scroll (#93) OpenTUI's to fit every item, so a quick pick with more items than the terminal has rows both overflowed the screen and could never actually scroll the selection into view. List now accepts an optional style (height/flexGrow/overflow) that a caller can use to bound it; with no style it keeps sizing to content exactly as before, so every existing unconstrained caller is unaffected. modalOverlay.tsx's QuickPickBody uses this to size List to however many rows actually fit below the modal's top margin (computed from useTerminalDimensions, reactive to live resizes) - still item-count-sized for a short list, capped once there are more items than fit. InputBoxBody's prompt/validation text can also overflow by wrapping across many rows with no scrollable widget underneath, so it gets a maxHeight + overflow: hidden clip instead. Adds a regression test that fails against the old unconditional-height code and a manual-verification step for long modals on a real terminal. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01WELSsojQQL1cTAR5iUUsTK --- docs/manual-release-verification.md | 32 +++++- packages/core/src/ui/components.tsx | 62 ++++++++++- packages/core/src/ui/modalOverlay.test.tsx | 87 ++++++++++++++- packages/core/src/ui/modalOverlay.tsx | 117 +++++++++++++++++++-- 4 files changed, 282 insertions(+), 16 deletions(-) diff --git a/docs/manual-release-verification.md b/docs/manual-release-verification.md index 0083e33..a15fc18 100644 --- a/docs/manual-release-verification.md +++ b/docs/manual-release-verification.md @@ -169,7 +169,33 @@ inside tmux): switch from the default theme to the other bundled theme (Dark Modern ↔ Light Modern). Confirm the whole UI repaints with the new palette immediately. -8. Exit cleanly (`ctrl+c` or the equivalent) and confirm the terminal is +8. **Long modals (issue #93 — "the display does not update when scrolling + within the modal")**: `modalOverlay.test.tsx`'s headless regression test + already proves the underlying layout math (a bounded `` (issue #93: "the display does not update when scrolling + * within the modal"). Deliberately just the handful of Yoga layout + * properties a bounding parent needs — not the full `SelectRenderableOptions` + * — since that is all `List`'s own callers (`modalOverlay.tsx`) have any + * business setting; every other renderable option (colors, `selectedIndex`, + * `focused`, …) stays owned by `List` itself, above. + */ +export interface ListStyle { + /** A fixed row count, or a percentage of the parent's height — the same + * two forms OpenTUI's own `height` accepts (`Renderable.d.ts`). */ + height?: number | `${number}%`; + /** Take a share of the parent's remaining flex space instead of a fixed + * size — e.g. `1` beside a `flexShrink: 0` sibling like the quick pick's + * filter `Input` (`modalOverlay.tsx`'s `QuickPickBody`). */ + flexGrow?: number; + /** Clip anything the underlying `` only ever scrolls + * — recentring `scrollOffset` on the selected index, per its vendored + * `SelectRenderable.updateScrollOffset` — when its assigned `height` is + * SMALLER than its option count; sized-to-content, `scrollOffset` can only + * ever resolve to `0`). Omitted (the default): `List` keeps sizing itself + * to fit every item exactly as before — this prop opts a caller INTO + * bounding it, so every pre-existing, unconstrained caller (including + * `components.snapshot.test.tsx`) renders byte-for-byte unchanged. + */ + style?: ListStyle; } /** A minimal selectable list (`tecode.ui.List`, Req 10.1), over OpenTUI's - * ``. + * + * **Sizing** (issue #93 fix): with no {@link ListProps.style}, `List` sizes + * its `` gets ONLY that bounded height/`flexGrow` — never the + * size-to-content one — letting OpenTUI's own `maxVisibleItems`/ + * `scrollOffset` machinery (verified against the vendored + * `@opentui/core@0.1.107` bundle's `SelectRenderable`) do its job: show a + * scrollable window and keep `selectedIndex` centred in it, exactly like + * every other bounded ` only shows as many rows as its own assigned // height, defaulting very small when unconstrained; size it to fit - // every item unless a parent layout (flexGrow, an explicit height) - // overrides this via `style`. - height={Math.max(items.length, 1)} + // every item unless the caller opted into a bounded `style` (this + // function's TSDoc's "Sizing"). + height={hasSizeConstraint ? undefined : Math.max(items.length, 1)} + style={props.style} selectedIndex={selectedIndex >= 0 ? selectedIndex : undefined} focused={props.focused} showDescription={items.some((item) => item.description)} diff --git a/packages/core/src/ui/modalOverlay.test.tsx b/packages/core/src/ui/modalOverlay.test.tsx index 4a3e16c..4953c02 100644 --- a/packages/core/src/ui/modalOverlay.test.tsx +++ b/packages/core/src/ui/modalOverlay.test.tsx @@ -15,7 +15,7 @@ import { createCommandRegistry } from "../commands/registry"; import { createHostLog } from "../host/errors"; import { ContextFocusTracker } from "./focus"; import { MODAL_DEFAULT_KEYBINDINGS, registerModalCommands } from "./modalCommands"; -import { ModalOverlay } from "./modalOverlay"; +import { modalMarginRows, ModalOverlay } from "./modalOverlay"; import { createModalService } from "./modalService"; import { ThemeProvider } from "./theme"; @@ -48,6 +48,35 @@ function findInputByPlaceholder( return undefined; } +/** Depth-first search for the quick pick's underlying OpenTUI `` to + // `Math.max(items.length, 1)` UNCONDITIONALLY — with 100 items in a + // 20-row terminal, the select's own assigned height was 100, so (a) it + // was laid out far past the terminal's bottom edge, and (b) OpenTUI's + // own `updateScrollOffset` can only ever resolve `scrollOffset` to `0` + // when `height >= options.length` (`components.tsx`'s TSDoc) — so it + // could never scroll a later item into view either. + const select = findSelect(renderer.root); + expect(select).toBeDefined(); + expect(select!.height).toBeLessThan(ITEM_COUNT); + expect(select!.y + select!.height).toBeLessThanOrEqual(TERMINAL_HEIGHT); + // And it isn't sized to some degenerate near-zero window either — this + // is a REAL, usable scrollable list, not just "technically bounded". + const availableRows = TERMINAL_HEIGHT - modalMarginRows(TERMINAL_HEIGHT); + expect(select!.height).toBeGreaterThan(0); + expect(select!.height).toBeLessThanOrEqual(availableRows); + + // The active item (index 0 initially) is the very first row — always + // trivially visible, bug or no bug. The real regression check is what + // happens once the SELECTION moves somewhere the OLD, unbounded layout + // would have drawn far below row 20. + expect(captureCharFrame()).toContain("Item 0"); + + // Move the active selection all the way to the LAST item. + for (let i = 0; i < ITEM_COUNT - 1; i++) { + act(() => modalService.selectNext()); + } + await act(async () => { + await renderOnce(); + }); + expect(modalService.getState()).toMatchObject({ mode: "quickPick", activeIndex: ITEM_COUNT - 1 }); + + const frame = captureCharFrame(); + expect(frame).toContain(`Item ${ITEM_COUNT - 1}`); + }); +}); diff --git a/packages/core/src/ui/modalOverlay.tsx b/packages/core/src/ui/modalOverlay.tsx index d7d3ef0..b0e5f1e 100644 --- a/packages/core/src/ui/modalOverlay.tsx +++ b/packages/core/src/ui/modalOverlay.tsx @@ -24,12 +24,36 @@ * `left`/`right`/`bottom` (resolved against the nearest sized ancestor — * here, the full-terminal root box `renderShell.tsx` wraps `` and * this component in), so centering needs no terminal-dimension hook or - * negative-margin arithmetic: `top: "15%", left: "15%", right: "15%"` - * removes the overlay from the normal flex flow and centers it - * horizontally with a 15%-of-width margin on each side, sized vertically by - * its own content. `zIndex` (a top-level `Renderable` option, confirmed in + * negative-margin arithmetic: `top`/`left`/`right`, all + * {@link MODAL_VERTICAL_MARGIN_PERCENT} (`"15%"`), remove the overlay from + * the normal flex flow and center it horizontally with a matching margin on + * each side. `zIndex` (a top-level `Renderable` option, confirmed in * `Renderable.d.ts`) lifts it above `Shell`'s own content. * + * **Vertical bound (issue #93 — "the display does not update when + * scrolling within the modal")**: this outer `` itself still has no + * `bottom`/`height` — it stays sized to whatever `QuickPickBody`/ + * `InputBoxBody` render, exactly as before this fix — but neither of THOSE + * any longer sizes itself unboundedly. The root cause (`components.tsx`'s + * `List` TSDoc): OpenTUI's ``, but a caller-supplied + * `prompt`/`validationMessage` that can ALSO overflow by wrapping across + * many rows) just gets a hard `maxHeight` + `overflow: "hidden"` clip, + * since it has no scrollable widget for a clamped size to make sense of. + * `useTerminalDimensions()` (`@opentui/react`, re-exported alongside this + * module's own `useRenderer` import) is what both read: reactive to the + * renderer's own `"resize"` event, so a live terminal resize re-bounds an + * already-open modal on its very next render, not just at mount. + * * **Focus save/restore, and the ordering hazard this module works around**: * {@link ModalOverlay} needs to remember whatever OpenTUI node was focused * immediately BEFORE a modal opens (it could be anything — the sidebar, the @@ -55,7 +79,7 @@ */ import { useCallback, useEffect, useReducer, useRef, type ReactNode } from "react"; -import { useRenderer } from "@opentui/react"; +import { useRenderer, useTerminalDimensions } from "@opentui/react"; import { Input, List, type ListItem } from "./components"; import type { FocusableNode } from "./focus"; import { useFocusTracking } from "./focus"; @@ -66,6 +90,45 @@ import { toColorInput, useTheme } from "./theme"; type QuickPickModalState = Extract; type InputBoxModalState = Extract; +/** + * Vertical margin (issue #93 — "the display does not update when scrolling + * within the modal"), in percent-of-terminal-height, kept on BOTH sides of + * the overlay's content — matches the `top: "15%"` this module's own outer + * `` (below) already uses for its top offset and its horizontal + * `left`/`right` margins, so the modal keeps the same visual proportions + * it always has; the number now ALSO seeds {@link modalMarginRows}, which + * bounds every mode's content height so it can never grow past the + * terminal (this module's TSDoc's "Positioning" already covers the + * horizontal half of this; nothing previously bounded the vertical half — + * see `List`'s `style`, `components.tsx`'s TSDoc, for why an OVERSIZED + * ``). Subtracted from + * the rows available below the top margin to get `List`'s own bound. */ +const QUICK_PICK_RESERVED_ROWS = 3; + /** Props for {@link ModalOverlay}. */ export interface ModalOverlayProps { /** Narrowed to exactly what rendering + input handling needs — matches @@ -123,6 +186,20 @@ function QuickPickBody(props: { })); const activeId = props.state.activeIndex >= 0 ? String(props.state.activeIndex) : undefined; + // Bound `List`'s height (issue #93's fix — `components.tsx`'s `List` + // TSDoc's "Sizing") to however many rows actually fit below the modal's + // own top margin, MINUS this box's own border (1 row top + 1 bottom — + // `border={[...]}` below) and the filter `Input` above (always exactly 1 + // row, single-line). Below that many items, this comes out to + // `listItems.length` itself — i.e. IDENTICAL to `List`'s own unconstrained + // default — so a short list still hugs its own content exactly as before; + // only once there are MORE items than fit does this clamp kick in, + // handing `List` a height smaller than its item count so OpenTUI's + // `` to size (issue #93 is specifically + // about `List`'s scrolling), but its `prompt`/`validationMessage` are + // caller-supplied strings of unbounded length — a long one wraps across + // many rows and can ALSO overflow the terminal, the same underlying + // "nothing bounds this modal's content" gap `QuickPickBody` closes above + // (verified empirically: a ~400-char prompt/validation pair in a + // 20-row terminal renders ~35 content rows, well past the bottom edge). + // A hard `maxHeight` + `overflow: "hidden"` clip — rather than + // `QuickPickBody`'s adaptive row-counting `listHeight` — is the right fix + // here: there is no scrollable widget underneath to keep a selection + // visible in, so there is nothing to make scrollable; clipping is simply + // "never draw past the terminal's edge", the same guarantee `List`'s own + // bounded `` specifically also broke scrolling, not just overflowed). */ -const MODAL_VERTICAL_MARGIN_PERCENT = 15; +const MODAL_MARGIN_PERCENT = 15; /** * A conservative (i.e. never UNDER-estimating) row count for one of this @@ -119,7 +122,7 @@ const MODAL_VERTICAL_MARGIN_PERCENT = 15; * guessing at Yoga's actual resolved pixel offset itself. */ export function modalMarginRows(terminalHeight: number): number { - return Math.ceil((terminalHeight * MODAL_VERTICAL_MARGIN_PERCENT) / 100); + return Math.ceil((terminalHeight * MODAL_MARGIN_PERCENT) / 100); } /** Rows {@link QuickPickBody}'s own chrome always occupies ABOVE `List`, @@ -327,9 +330,9 @@ export function ModalOverlay(props: ModalOverlayProps): ReactNode { From bcfc8176f26e81a90e382ed5b5588b84ff7bbfb3 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 07:59:37 +0000 Subject: [PATCH 3/4] Pin that a long input-box prompt cannot hide the input itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review raised the worry that InputBoxBody's maxHeight + overflow clip could push the Input and the validation message out through the bottom edge behind a long enough prompt, leaving a modal that takes keystrokes it cannot show. Measured instead of assumed: a 468-character prompt in a 40x20 terminal truncates and leaves both on screen. Adding an explicit flexShrink: 1 to the prompt produced a byte-identical frame, so that configuration was dropped rather than kept as a no-op with a comment claiming it prevented something. The test is therefore not a regression test — nothing needed fixing. It pins the behaviour the clip already has so a future layout change cannot quietly take it away. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01WELSsojQQL1cTAR5iUUsTK --- packages/core/src/ui/modalOverlay.test.tsx | 41 ++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/packages/core/src/ui/modalOverlay.test.tsx b/packages/core/src/ui/modalOverlay.test.tsx index 4953c02..295b93b 100644 --- a/packages/core/src/ui/modalOverlay.test.tsx +++ b/packages/core/src/ui/modalOverlay.test.tsx @@ -359,3 +359,44 @@ describe("ModalOverlay — long quick picks stay bounded and scrollable (issue # expect(frame).toContain(`Item ${ITEM_COUNT - 1}`); }); }); + +describe("ModalOverlay — a long input-box prompt never hides the input itself", () => { + // NOT a regression test: no code change was needed to make this pass. + // Review raised the worry that `InputBoxBody`'s `maxHeight` + + // `overflow: "hidden"` clip could push the `Input` and the validation + // message out through the bottom edge behind a long enough prompt, + // leaving a modal that takes keystrokes it cannot show. Rendering a + // 468-character prompt in a 40x20 terminal shows it does not: the prompt + // truncates and both stay on screen. Adding an explicit `flexShrink: 1` + // to the prompt produced a byte-identical frame, so that configuration + // was dropped rather than kept as a no-op. This test pins the behaviour + // the clip already has, so a future layout change cannot quietly take it + // away. + test("a prompt long enough to overflow the clip still leaves the typed value and validation message on screen", async () => { + const TERMINAL_WIDTH = 40; + const TERMINAL_HEIGHT = 20; + // 468 characters — long enough to wrap well past the modal's own + // clipped height at this width. + const prompt = "This prompt is deliberately very long. ".repeat(12); + const modalService = createModalService(); + void modalService.openInputBox({ prompt, validateInput: () => "VALIDATION_SENTINEL" }); + modalService.setInputValue("TYPED_SENTINEL"); + + const { renderOnce, captureCharFrame } = await testRender( + + + , + { width: TERMINAL_WIDTH, height: TERMINAL_HEIGHT }, + ); + await act(async () => { + await renderOnce(); + }); + + const frame = captureCharFrame(); + // A modal that accepts keystrokes must show them. Losing some of the + // prompt to the clip is the acceptable trade; losing the field the user + // is typing into would not be. + expect(frame).toContain("TYPED_SENTINEL"); + expect(frame).toContain("VALIDATION_SENTINEL"); + }); +}); From 219f64b3039172f75bb365c1a2aef64865a78722 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 08:01:28 +0000 Subject: [PATCH 4/4] Rewrite the garbled sentence in the long-modal verification step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "— resize this test for, and the whole reason this bug shipped in the first place" was not a sentence. Replaced with what it was reaching for: the headless test inspects one captured frame per render while the bug users hit was a stale screen across many, and it fixes the terminal size up front so it cannot cover a live resize. Those two gaps are why the layout being testable did not stop this shipping, which is what makes the manual check worth doing. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01WELSsojQQL1cTAR5iUUsTK --- docs/manual-release-verification.md | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/docs/manual-release-verification.md b/docs/manual-release-verification.md index a15fc18..05278b5 100644 --- a/docs/manual-release-verification.md +++ b/docs/manual-release-verification.md @@ -171,14 +171,17 @@ inside tmux): palette immediately. 8. **Long modals (issue #93 — "the display does not update when scrolling within the modal")**: `modalOverlay.test.tsx`'s headless regression test - already proves the underlying layout math (a bounded `` that + fits the terminal and keeps the active item inside its visible window + (`modalOverlay.tsx`'s TSDoc's "Vertical bound"). What it cannot prove is + that a real terminal actually REPAINTS that window as the selection + moves: the headless test inspects one captured frame per render, while + the bug users hit was a stale-looking screen across many. Nor can it + cover a live resize, since it fixes the terminal size up front. Those + two gaps are why this bug shipped despite the layout being testable, so + check them by hand. In a directory/project with enough files and + commands to overflow the terminal's height (or simply shrink the + terminal window first): 1. Open `workbench.action.quickOpen` (`ctrl+p`). Confirm the picker's box stops well short of the terminal's bottom edge — no filenames spill past it or get cut off mid-row — and that holding `down`