Bound the slash-command and provider-setup pickers so they scroll instead of overflowing - #370
Conversation
Every direct child of the setup screen's root column needs flexShrink: 0. header, intro, step, instruction, statusLine, guidance, and footer were all missing it while every sibling box already had it, so a terminal too short for the full column let the flex algorithm compress these unprotected single-line rows onto each other instead of clipping from the bottom. Reproduces on the provider pick-list and independently on the failed-connection-test screen, where statusLine and guidance are populated together. The list height budget also reserved more chrome rows than the picker actually uses, so the footer could go missing even on terminals that had room for it.
Investigation found the picker already routes through the shared list-viewport/resolveGeometry machinery: a 50-entry catalog stays bounded at 24, 16, 12, 8, and 6 terminal rows, the prompt box below it stays intact, and moving the selection scrolls the window so the active row is always visible. No sizing logic was missing, so this adds the regression coverage without a second windowing path.
e0544b2 to
dd66dad
Compare
Review summary — two rounds, held open for human reviewReady for your call. Not merged. CL-5363 — the real fixThe provider setup screen garbled text on short terminals because rows added as direct children of The first round fixed Both rounds are now in. Independently verified: all twelve direct children of Regression coverage now drives the flow through provider pick → API key → failed connection test, reaching the state the first round's tests missed. CL-5412 — no source change, coverage onlyIndependently confirmed: the branch's Other renderers ruled out with evidence
Architectural note, filed separately as CL-5599Routing The narrower convergence is a shared helper computing the chrome budget from an explicit list of fixed rows. That matters beyond tidiness — it would have surfaced the missing Typecheck and build clean. 4050 pass / 0 fail, identical under |
listHeight() used a flat constant for the rows above and below the pick-list, re-guessed by hand whenever a row was added or removed. Compute it from the same building blocks the tree is made of instead: the seven always-visible single-line rows, root and listBox padding, the summary box's per-step rows, and the telemetry notice when shown.
The derived chromeRows computation (singleLineRows, rootPadding, listBoxPadding, summaryRows) reads as principled but is still a hand-maintained count of the layout, now spread across four constants instead of one. Nothing catches it going stale when a row is added, since the tests cover height bounding, not the count, and the Math.max hedge covers a PRESET/OAUTH steps divergence that does not exist. Restored rows - 14 with a comment stating plainly that it is a guess: listHeight() runs before root is constructed, so there is no OpenTUI layout pass yet to measure a real chrome height from. Renderable.height and scrollHeight only reflect the last completed layout, populated post-mount, so they cannot help here. A shared, derived chrome budget for this and shell.ts's picker is tracked separately.
1 participant
Summary
Both tickets turned out to have different root causes, not one shared bug:
same shared
resolveGeometry/createListViewport/visibleSlicemachinery as the
@mention picker and every other list overlay inshell.ts. A 50-entry catalog stays bounded and scrolls correctly atevery terminal height tested. This PR adds the regression coverage the
ticket asked for; no source change.
provider-setup.tsis a standalone full-screen renderer that does notgo through
resolveGeometry— it owns the whole terminal directly withits own flex-column layout and its own
listHeight()budget. Two defects:flexShrink: 0.header,intro,step,instruction,statusLine,guidance,and
footerwere all missing it while every sibling box already hadit. On a short terminal the flex algorithm compressed these
unprotected single-line rows into each other instead of clipping from
the bottom — visible as garbled, overlapping text. First caught on
the provider pick-list (
stepe1tofn3i·fproviderrovider — switch later with /model); a first pass fixed onlyheader/intro/step/instructionand missed thatstatusLine/guidance/footerhadthe identical omission, which reproduces independently on the
failed-connection-test screen once both rows are populated. All
seven are now covered.
listHeight()reserved a flatrows - 14for chrome, which is morethan the picker step actually costs, so the list sat shorter than it
needed to and the footer could still go missing on terminals that had
room for it. Tightened to a documented
-12; still a hand-derivedconstant, not a computed one — see the follow-up note below.
landing.ts/stream.ts: checked for the same class of bug, foundnothing to fix.
landing.ts's boxes are mounted as children ofshell.ts'sresolveGeometry-managed root (viacreateLandingAbove/createLandingBelow), not a standalone tree, so they inherit theshell's chrome-collapse discipline instead of doing their own; rendered
at 24/16/12/8/6 rows and it clips cleanly with no overlap at every size.
stream.tshas no renderable tree of its own at all — it's row-layoutmath consumed by
shell.ts's (also geometry-managed) transcriptpainting. Neither is a second instance of
provider-setup.ts'sstandalone-layout risk.
Net effect: +166 / -1 lines. All of the growth is test coverage (three
new
describe/test blocks); the fix itself is a 22-line diff inprovider-setup.ts.Why not one shared fix
CL-5412's picker and CL-5363's picker are not the same code path. The
shell's inset overlays (mentions, palette/slash, permissions) share one
mechanism;
provider-setup.tsis a dedicated onboarding/full-screensurface with its own layout because it runs before a shell exists. Routing
it through
resolveGeometrywould mean threading a shell-shaped zonemodel into a screen that isn't part of the shell's zone system — a much
larger, riskier change for the same outcome the existing
list-viewport.tsmachinery it already uses gives it directly. The actualbug was a missing
flexShrink: 0and an over-conservative constant, not amissing abstraction, so the fix stays local.
Follow-up (not built here, filed separately)
The narrower convergence between this file and
shell.tsis a smallshared helper that computes a chrome budget from an explicit list of
fixed rows, called from both places. That would make the
-12here aderived value instead of a hand-tuned guess — and, more importantly, it
would have surfaced the missing
flexShrinkonstatusLine/guidance/footeras "these rows aren't in the fixed-row list" at write timeinstead of compiling silently and shipping. Routing
provider-setup.tsthrough the shell's
resolveGeometryitself was considered and rejected:that resolver operates over the shell's
ZoneIdheight model, and thisscreen runs before a shell exists, so adopting it would mean standing up
a degenerate shell or inventing a second zone taxonomy — a real cost, not
an excuse. The bounded-helper version is the one worth doing.
Changes
src/tui-opentui/provider-setup.ts: addflexShrink: 0to all sevenchrome text rows (
header,intro,step,instruction,statusLine,guidance,footer); tightenlistHeight()'s chromebudget from a flat
-14to a documented-12.src/tui-opentui/provider-setup.test.ts: bounded-frame + no-overlapregression tests at 24/16/12/8/6 rows, a keyboard-scroll test, and a
failed-connection-test regression that walks into the
statusLine+guidancestate where the second omission was found.src/tui-opentui/palette-paint.test.ts: bounded-frame + scroll-windowregression tests for the slash/command palette at the same five heights,
against a 50-entry fake catalog.
Verification
bun run typecheck- cleanbun run build- cleanbun run test(canonical,./src ./tests ./evals) - 4050 pass, 0 failbun test ./src ./tests ./evals --randomize --seed 42- 4050 pass, 0 fail12, 8, and 6 terminal rows, with a 40+ entry list, via the existing
withTestRenderer/createHarnessharnesses, and forlanding.tsatthe same five heights to rule it out.
Slash/command palette (50 fake commands), before scroll
After pressing Down 20 times at height=12 (window scrolls, not just the
cursor):
overlayList height=1 offset=20 active=20 count=50-- the window followsthe active row exactly.
Agent configuration / provider picker, before fix (height=12)
Garbled: the step line and the intro line were compressed onto one row.
Agent configuration / provider picker, after fix
Example, height=24 after the fix:
Provider picker, failed-connection-test state (height=16, after fix)
The state that exposed the second
flexShrinkomission: statusLine andguidance are both populated at once, and now render as two clean rows
instead of merging into each other.
Closes CL-5412
Closes CL-5363