Skip to content

Commit c10b5b6

Browse files
LaZzyManLaZzyMan
committed
fix(cli): wrap-aware warnings reservation + load-bearing regression test (#4386 self-review)
Two related self-review findings from my pre-push review on PR #4386: 1. SR-1: the round-1 layout regression test (`keeps options visible alongside the warning on a tight compactMode layout`) used a single-line command at `contentWidth=80`. `MaxSizedBox` clamps to `min(content_lines, maxHeight)`, so a 1-line command renders as 1 line regardless of whether `bodyContentHeight -= warningsHeight` is applied — the test would still pass if that line were silently removed. Replaced with a 4-line command and an assertion on the `... N lines hidden ...` truncation footer that MaxSizedBox emits *only* when its cap is actually active. Confirmed RED → fix → GREEN: the new test fails when the warnings subtraction is removed and passes when restored. 2. SR-2: the warnings-height reservation reserved `warningsCount + 1` lines (one per warning + one marginTop separator). On narrow terminals the warning text wraps across multiple visual rows, so the reservation under-counts. Replace the per-warning `+1` with `ceil((warning.length + 2) / max(contentWidth, 1))` to account for each warning's actual rendered row count. The `+ 2` accounts for the `⚠ ` prefix; the `max(...,1)` keeps the math defined for pathological inputs. Independently flagged by Codex during self-review as `[P3] Account for wrapped warning lines`.
1 parent 7bb4205 commit c10b5b6

2 files changed

Lines changed: 52 additions & 13 deletions

File tree

‎packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx‎

Lines changed: 33 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -119,17 +119,36 @@ describe('ToolConfirmationMessage', () => {
119119
expect(lastFrame() ?? '').not.toContain('command substitution');
120120
});
121121

122-
// Regression coverage for the round-1 review on PR #4386: the warnings
123-
// block sits outside the MaxSizedBox cap, so its footprint has to be
124-
// reserved up-front; otherwise the options list can be pushed off-screen
125-
// on small terminals. Verify that on a tight compactMode layout the
126-
// warning text AND every option label still render.
127-
it('keeps options visible alongside the warning on a tight compactMode layout', () => {
122+
// Regression coverage for the round-1 review on PR #4386 (PR #4386 round-2
123+
// self-review SR-1): the warnings block sits outside the MaxSizedBox
124+
// cap, so its footprint has to be reserved from `bodyContentHeight`
125+
// up-front; otherwise the options list can be pushed off-screen on
126+
// small terminals. The original round-1 test used a single-line
127+
// command, which made the `MaxSizedBox` clamp `min(content_lines,
128+
// maxHeight)` reduce to `min(1, X) = 1` regardless of the
129+
// reservation — i.e. the test was vacuous. Replaced here with a
130+
// multi-line command so the clamp is actually exercised, and the
131+
// assertion checks for the `... N lines hidden ...` truncation
132+
// footer that MaxSizedBox emits ONLY when its cap is active. Without
133+
// the warnings reservation, the cap is loose enough that the whole
134+
// command fits and the footer never appears.
135+
it('clamps the multi-line command body to make room for the warning on a tight compactMode layout', () => {
136+
// Four-line command: forces MaxSizedBox to clamp once the warnings
137+
// footprint is reserved. With the reservation: cap is tight enough
138+
// that the body is truncated and shows a "... N lines hidden ..."
139+
// footer. Without it: the whole 4-line command renders and the
140+
// footer is absent.
141+
const command = [
142+
'cmd-line-1',
143+
'cmd-line-2',
144+
'cmd-line-3',
145+
'cmd-line-4',
146+
].join('\n');
128147
const confirmationDetails: ToolCallConfirmationDetails = {
129148
type: 'exec',
130149
title: 'Confirm Shell Command',
131-
command: 'python3 -c "print($(echo hello))"',
132-
rootCommand: 'python3',
150+
command,
151+
rootCommand: 'cmd-line-1',
133152
warnings: [
134153
'Contains command substitution ($(...), backticks, <(...), or >(...)).',
135154
],
@@ -147,9 +166,13 @@ describe('ToolConfirmationMessage', () => {
147166
);
148167

149168
const frame = lastFrame() ?? '';
150-
// Warning is the load-bearing UX add — must not be truncated.
169+
// MaxSizedBox emits this footer when it clamps; its presence proves
170+
// the reservation actually narrowed the body cap below the command
171+
// height. Without `bodyContentHeight -= warningsHeight`, the cap is
172+
// loose and the footer doesn't appear.
173+
expect(frame).toMatch(/lines hidden/);
174+
// Warning + all three compactMode options must still be on-screen.
151175
expect(frame).toContain('command substitution');
152-
// All three compactMode options must still be on-screen.
153176
expect(frame).toContain('Yes, allow once');
154177
expect(frame).toContain('Allow always');
155178
expect(frame).toContain('No');

‎packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -256,9 +256,25 @@ export const ToolConfirmationMessage: React.FC<
256256
// overall exec block can exceed availableTerminalHeight /
257257
// COMPACT_BODY_MAX_LINES on small terminals and push the options
258258
// list off-screen.
259-
const warningsCount = executionProps.warnings?.length ?? 0;
260-
// 1 line per warning + 1 line for the marginTop separator.
261-
const warningsHeight = warningsCount > 0 ? warningsCount + 1 : 0;
259+
//
260+
// Each warning may wrap across multiple visual rows on a narrow
261+
// terminal. Account for that by computing `ceil(rendered_len /
262+
// contentWidth)` per warning (rendered length includes the leading
263+
// `⚠ ` glyph + space, so add 2). Falling back to a 1-row estimate
264+
// when contentWidth is non-positive keeps the math defined for
265+
// pathological inputs.
266+
const warningPrefixLen = 2; // "⚠ "
267+
const safeWidth = Math.max(contentWidth, 1);
268+
const warnings = executionProps.warnings ?? [];
269+
const warningsCount = warnings.length;
270+
const wrappedWarningRows = warnings.reduce(
271+
(sum, w) =>
272+
sum + Math.max(Math.ceil((w.length + warningPrefixLen) / safeWidth), 1),
273+
0,
274+
);
275+
// wrapped rows + 1 line for the marginTop separator (only when at
276+
// least one warning is present).
277+
const warningsHeight = warningsCount > 0 ? wrappedWarningRows + 1 : 0;
262278

263279
let bodyContentHeight = availableBodyContentHeight();
264280
if (bodyContentHeight !== undefined) {

0 commit comments

Comments
 (0)