Bound the modal's height so long lists scroll instead of clipping

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 `<select>` 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`
+ through every item visibly SCROLLS the list (the window of visible
+ rows moves) with the highlighted row always on screen, all the way
+ to the last item.
+ 2. Open `workbench.action.showCommands` (`ctrl+shift+p`) and repeat the
+ same check against the command list.
+ 3. Run `keybindings.showResolved` (via the command palette — Req
+ 11.7/design.md §13, it has no keybinding of its own) against a
+ keymap with enough bindings to exceed the terminal's height; repeat
+ the same check.
+ 4. Shrink the terminal window to a noticeably smaller size WHILE one of
+ the pickers above is still open, and confirm the picker re-bounds
+ itself to the new size on the next redraw rather than staying
+ pinned to the old (now possibly too-large) box.
+9. Exit cleanly (`ctrl+c` or the equivalent) and confirm the terminal is
  restored to its normal (non-raw, non-alternate-screen) state.
-9. Record which terminal emulator and OS/arch combination was used, plus a
- screenshot or terminal recording, in the PR.
+10. Record which terminal emulator and OS/arch combination was used, plus a
+screenshot or terminal recording, in the PR.

packages/core/src/ui/components.tsx

@@ -72,16 +72,68 @@ export interface ListItem {
  description?: string;
 }
 
+/**
+ * A caller-supplied size constraint for {@link List}'s underlying
+ * `<select>` (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 `<select>` would otherwise draw past its
+ * own bounds — belt-and-suspenders alongside a bounded `height`/
+ * `flexGrow` above; `SelectRenderable` already stops drawing options past
+ * its own `height` on its own (`components.tsx`'s TSDoc's "Sizing"), so
+ * this is defense-in-depth, not load-bearing. */
+ overflow?: "visible" | "hidden" | "scroll";
+}
+
 /** {@link List}'s props. */
 export interface ListProps {
  items?: ListItem[];
  selectedId?: string;
  onSelect?: (id: string) => void;
  focused?: boolean;
+ /**
+ * Bounds `List`'s own height instead of letting it grow to fit every
+ * item (issue #93's root cause: OpenTUI's `<select>` 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
- * `<select>`. */
+ * `<select>`.
+ *
+ * **Sizing** (issue #93 fix): with no {@link ListProps.style}, `List` sizes
+ * its `<select>` to fit every item (`height={Math.max(items.length, 1)}`,
+ * unchanged from before this fix) — the right default for a caller that
+ * already knows its list is short (e.g. the keybindings editor's fixed
+ * panes) and wants no scrolling machinery at all. A caller expecting an
+ * UNBOUNDED item count (the command palette / quick-open / any
+ * `showQuickPick`, via `modalOverlay.tsx`) passes `style` instead, so the
+ * `<select>` 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 `<select>` in the ecosystem.
+ */
 export function List(rawProps: Record<string, unknown>): ReactNode {
  const props = rawProps as ListProps;
  const theme = useTheme();
@@ -94,15 +146,17 @@ export function List(rawProps: Record<string, unknown>): ReactNode {
  const selectedIndex = props.selectedId
  ? items.findIndex((item) => item.id === props.selectedId)
  : -1;
+ const hasSizeConstraint = props.style !== undefined;
 
  return (
  <select
  options={options}
  // OpenTUI's <select> 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)}

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 `<select>`
+ * renderable — found by its distinctive method signature
+ * (`getSelectedIndex`/`setSelectedIndex`) rather than `instanceof
+ * SelectRenderable` (`@opentui/core` isn't otherwise imported as a runtime
+ * value here) or a `constructor.name` string check (fragile under any
+ * future minification of the vendored bundle). Exposes the real, laid-out
+ * `y`/`height` this suite's regression test needs — `List`'s own React
+ * props (`components.tsx`) don't reveal what OpenTUI's Yoga layout actually
+ * resolved them to. */
+function findSelect(
+ node: unknown,
+): { y: number; height: number; getSelectedIndex(): number; getChildren?: () => unknown[] } | undefined {
+ const candidate = node as {
+ getSelectedIndex?: () => number;
+ setSelectedIndex?: (index: number) => void;
+ y?: number;
+ height?: number;
+ getChildren?: () => unknown[];
+ };
+ if (typeof candidate?.getSelectedIndex === "function" && typeof candidate?.setSelectedIndex === "function") {
+ return candidate as { y: number; height: number; getSelectedIndex(): number; getChildren?: () => unknown[] };
+ }
+ for (const child of candidate?.getChildren?.() ?? []) {
+ const found = findSelect(child);
+ if (found) return found;
+ }
+ return undefined;
+}
+
 describe("ModalOverlay — rendering", () => {
  test("renders nothing while no modal is open", async () => {
  const modalService = createModalService();
@@ -274,3 +303,100 @@ describe("ModalOverlay — focus save/restore (Req 10.1)", () => {
  expect(renderer.currentFocusedRenderable).toBe(priorNode as unknown as BoxRenderable);
  });
 });
+
+describe("ModalOverlay — long quick picks stay bounded and scrollable (issue #93 regression)", () => {
+ test("far more items than the terminal has rows: the select's rendered rows fit inside the terminal, and the active item is inside its visible window", async () => {
+ const TERMINAL_WIDTH = 80;
+ const TERMINAL_HEIGHT = 20;
+ const ITEM_COUNT = 100;
+ const items = Array.from({ length: ITEM_COUNT }, (_, i) => ({ label: `Item ${i}` }));
+ const modalService = createModalService();
+ void modalService.openQuickPick(items);
+
+ const { renderOnce, renderer, captureCharFrame } = await testRender(
+ <ThemeProvider>
+ <ModalOverlay modalService={modalService} />
+ </ThemeProvider>,
+ { width: TERMINAL_WIDTH, height: TERMINAL_HEIGHT },
+ );
+ await act(async () => {
+ await renderOnce();
+ });
+
+ // The bug (issue #93): `List` used to size its `<select>` 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(
+ <ThemeProvider>
+ <ModalOverlay modalService={modalService} />
+ </ThemeProvider>,
+ { 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");
+ });
+});