diff --git a/docs/manual-release-verification.md b/docs/manual-release-verification.md index 0083e33..05278b5 100644 --- a/docs/manual-release-verification.md +++ b/docs/manual-release-verification.md @@ -169,7 +169,36 @@ 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..295b93b 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}`); + }); +}); + +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"); + }); +}); diff --git a/packages/core/src/ui/modalOverlay.tsx b/packages/core/src/ui/modalOverlay.tsx index d7d3ef0..af05a08 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_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,48 @@ 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 — one number for BOTH axes, which is why it is + * not named `..._VERTICAL_...`: changing it to tune the vertical inset + * moves the horizontal one too, by design. 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 +189,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 `