fix(cli): prevent selection dialog flicker - #4755
Conversation
| </Box> | ||
| </Box> | ||
| ) : ( | ||
| <Box flexDirection="column" marginBottom={1} flexShrink={1}> |
There was a problem hiding this comment.
[Suggestion] The constrained branch drops the "Shell Command Execution" title and "A custom command wants to run the following shell commands:" subtitle entirely. Users in small terminals lose context about what subsystem is requesting permission — the dialog shows only a truncated command list and "Do you want to proceed?".
Consider always rendering the title and subtitle above the ternary, and only switching the command list rendering between MaxSizedBox and the unbounded list. Adjust SHELL_CONFIRMATION_FIXED_ROWS to account for the always-visible title rows.
— qwen3.7-max via Qwen Code /review
| const { dialogOpen: bgTasksDialogOpen } = useBackgroundTaskViewState(); | ||
| const { constrainHeight, terminalHeight, staticExtraHeight, mainAreaWidth } = | ||
| uiState; | ||
| const dialogMaxHeight = Math.max(1, terminalHeight - staticExtraHeight - 2); |
There was a problem hiding this comment.
[Suggestion] The formula Math.max(1, terminalHeight - staticExtraHeight - 2) is independently computed in three places (here, DefaultAppLayout, and ScreenReaderAppLayout). The -2 safety margin is undocumented — future maintainers cannot determine what UI chrome it represents. Additionally, SettingsDialog, StatusLineDialog, and BackgroundTasksDialog receive dialogMaxHeight unconditionally (lines 264, 279, 505), ignoring the constrainHeight flag while all other dialogs use constrainedDialogHeight.
Consider extracting a shared utility with a comment explaining the -2 (e.g. // 1 row for top border + 1 row for bottom border/ExitWarning overlap), and using constrainedDialogHeight consistently for all dialogs or documenting why these three must always be constrained.
— qwen3.7-max via Qwen Code /review
| const maxModeItemsToShow = | ||
| constrainedHeight === undefined | ||
| ? DEFAULT_MAX_MODE_ITEMS_TO_SHOW | ||
| : Math.max( |
There was a problem hiding this comment.
[Suggestion] modeListChromeHeight accounts for chrome, spacer, and footer hint rows but omits the showWorkspacePriorityWarning box (~2 rows: marginTop={1} + wrapping <Text>). When the warning is visible in a constrained terminal, the footer keyboard hint or the warning itself may be silently clipped by overflow="hidden".
Add the warning's row count to the budget:
const warningRows = showWorkspacePriorityWarning ? 2 : 0;
const modeListChromeHeight =
MODE_LIST_CHROME_ROWS +
(showModeSpacer ? MODE_SPACER_ROWS : 0) +
(showFooterHint ? FOOTER_HINT_ROWS : 0) +
warningRows;— qwen3.7-max via Qwen Code /review
| isFeedbackDialogOpen: false, | ||
| mainAreaWidth: 80, | ||
| terminalWidth: 80, | ||
| terminalHeight: 24, |
There was a problem hiding this comment.
[Suggestion] ScreenReaderAppLayout.tsx receives the same dialogMaxHeight / dialogHeight / overflow changes as DefaultAppLayout.tsx, but no new tests verify the height-constraint behavior. DefaultAppLayout.test.tsx adds two tests (constrained + unconstrained); this file adds only base-UIState fields.
Screen-reader users are the audience most harmed by a layout regression here. The vi.hoisted DialogManager mock pattern from DefaultAppLayout.test.tsx can be reused directly.
— qwen3.7-max via Qwen Code /review
…-flash # Conflicts: # packages/cli/src/ui/components/ApprovalModeDialog.tsx
Verification ReportBranch: Test Results
Code Review Notes
Verdict✅ Ready to merge — all 58 tests pass across 7 test files, lint is clean, build succeeds. No typecheck errors in any PR-touched files. The implementation correctly constrains dialog height while preserving unconstrained mode as the default. Verified by wenshao |
| marginX={2} | ||
| flexDirection="column" | ||
| width={uiState.mainAreaWidth} | ||
| height={dialogHeight} |
There was a problem hiding this comment.
[Critical] The outer layout Box clips the bottom of inner dialogs because Ink uses border-box height.
Ink's height includes border rows (verified empirically: a Box with height={4} + borderStyle="round" renders exactly 4 total rows = 2 border + 2 content). This outer Box clips its children at dialogHeight rows, but each inner dialog (e.g. ShellConfirmationDialog with borderStyle="round") occupies dialogHeight + 2 total rows. The bottom 2 rows — where the radio button options render — are silently clipped.
Concrete impact: with staticExtraHeight=3 and DIALOG_VERTICAL_SAFETY_MARGIN=2, the dialog receives constrainedHeight = terminalHeight - 5. At a 15-row terminal (common in split panes and tmux), constrainedHeight = 10: the "No (esc)" option in ShellConfirmationDialog is invisible. At a 13-row terminal, only "Yes, allow once" is visible. The same issue exists in ScreenReaderAppLayout.tsx.
The existing tests don't catch this because ShellConfirmationDialog.test.tsx renders the dialog directly (no outer Box), so the outer clip is never exercised.
| height={dialogHeight} | |
| overflow={uiState.constrainHeight ? 'hidden' : undefined} |
Remove height={dialogHeight} from the outer Box. Each dialog already receives availableTerminalHeight={constrainedDialogHeight} and self-constrains via its own height + overflow="hidden". The outer Box's height is redundant and causes the double-clipping. Alternatively, if a safety net is desired, reduce the outer height to account for the dialog border: height={dialogHeight !== undefined ? dialogHeight + 2 : undefined}.
— qwen3.7-max via Qwen Code /review
| const commandPreviewHeight = | ||
| constrainedHeight === undefined | ||
| ? undefined | ||
| : Math.max(2, constrainedHeight - SHELL_CONFIRMATION_FIXED_ROWS); |
There was a problem hiding this comment.
[Suggestion] The Math.max(2, ...) floor forces the command preview to at least 2 rows even when the terminal is too small. This adds 2 rows that compete with the radio buttons for vertical space, causing the options (including "No (esc)") to be clipped at constrainedHeight < 11 (terminal height < 16 rows).
The command preview is informational; the radio buttons are the actionable part and should never be sacrificed for it.
| : Math.max(2, constrainedHeight - SHELL_CONFIRMATION_FIXED_ROWS); | |
| : constrainedHeight >= SHELL_CONFIRMATION_FIXED_ROWS + 2 | |
| ? Math.max(2, constrainedHeight - SHELL_CONFIRMATION_FIXED_ROWS) | |
| : 0; |
Then in the JSX, skip MaxSizedBox when commandPreviewHeight is 0:
{commandPreviewHeight ? (
<Box flexDirection="column" flexShrink={1}>
<MaxSizedBox maxHeight={commandPreviewHeight} ...>
...
</MaxSizedBox>
</Box>
) : null}This ensures the radio buttons always have room when the terminal is small.
— qwen3.7-max via Qwen Code /review
| * painting into the terminal's bottom rows before control-height measurement | ||
| * settles. | ||
| */ | ||
| export const getDialogMaxHeight = ( |
There was a problem hiding this comment.
[Suggestion] DIALOG_VERTICAL_SAFETY_MARGIN = 2 is coupled to a bare - 2 literal in AppContainer.tsx (terminalHeight - controlsHeight - staticExtraHeight - 2 - tabBarHeight) only by this JSDoc comment. There is no shared constant or import linking the two values. If someone adds a status bar row or changes staticExtraHeight in AppContainer, the dialog height budget silently becomes wrong — the regression is purely visual and only shows up at specific terminal heights.
Consider extracting the AppContainer-side reservation into a named constant (e.g., MAIN_CONTENT_HEIGHT_RESERVATION) in a shared location and importing it in both AppContainer.tsx and layoutUtils.ts, so the coupling is structural rather than comment-only.
— qwen3.7-max via Qwen Code /review
| expect(lastFrame() ?? '').toContain('Automatically approve all tools'); | ||
| }); | ||
|
|
||
| it('keeps the workspace priority warning visible when constrained', () => { |
There was a problem hiding this comment.
[Suggestion] This test at height 12 verifies both the workspace warning and footer hint are visible. But at heights 10–11 with showWorkspacePriorityWarning = true, showFooterHint becomes false (because constrainedHeight < MIN_HEIGHT_WITH_WARNING_FOOTER_HINT = 12). This footer-hiding branch — which reclaims 2 rows for the mode list — is never exercised.
Consider adding a test:
it('hides the footer hint to make room for the workspace warning', () => {
const { lastFrame } = renderWithProviders(
<ApprovalModeDialog
settings={createSettings({ tools: { approvalMode: ApprovalMode.YOLO } })}
currentMode={ApprovalMode.DEFAULT}
availableTerminalHeight={10}
onSelect={vi.fn()}
/>,
);
const frame = lastFrame() ?? '';
expect(frame).toContain('Workspace approval mode exists');
expect(frame).not.toContain('Use Enter to select');
});— qwen3.7-max via Qwen Code /review
|
Follow-up after the latest review:
Verification run locally:
|
|
Additional follow-up after rebasing onto the latest
Verification run locally after the upstream merge and fix:
|
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Critical] SkillsManagerDialog in DialogManager.tsx:444 still uses the inline formula constrainHeight ? terminalHeight - staticExtraHeight : undefined instead of the centralized constrainedDialogHeight (which subtracts MAIN_CONTENT_HEIGHT_RESERVATION = 2 via getDialogMaxHeight()). Every other height-constrained dialog in this file was migrated — SkillsManagerDialog is the sole holdout, receiving a height budget 2 rows larger than the parent layout Box actually allocates.
availableTerminalHeight={constrainedDialogHeight}
— qwen3.7-max via Qwen Code /review
| @@ -455,7 +455,13 @@ class SkillToolInvocation extends BaseToolInvocation<SkillParams, ToolResult> { | |||
| // makes the system MORE fragile to MCP failures, not less. | |||
| try { | |||
| const content = await this.commandExecutor(this.params.skill); | |||
There was a problem hiding this comment.
[Critical] The disabled-skill fallback calls this.commandExecutor(this.params.skill) without passing this.params.args. The non-disabled path at line 498 passes this.params.args ?? ''. When a disabled skill delegates to an MCP prompt or file command, the model-provided arguments are silently discarded.
| const content = await this.commandExecutor(this.params.skill); | |
| const content = await this.commandExecutor(this.params.skill, this.params.args ?? ''); |
— qwen3.7-max via Qwen Code /review
| } | ||
|
|
||
| const DEFAULT_MAX_MODE_ITEMS_TO_SHOW = 10; | ||
| const CONSTRAINED_DIALOG_HEIGHT_THRESHOLD = 12; |
There was a problem hiding this comment.
[Suggestion] CONSTRAINED_DIALOG_HEIGHT_THRESHOLD and the derived isVerticallyConstrained flag never change behavior. Tracing all possible constrainedHeight values: for >= 12, isVerticallyConstrained is false so both showModeSpacer and showFooterHint are unconditionally true; for 9–11, the individual threshold checks (>= MIN_HEIGHT_WITH_MODE_SPACER, >= MIN_HEIGHT_WITH_FOOTER_HINT) already return true; for < 9, both branches agree. The flag is dead code that adds cognitive overhead.
Simplify by removing CONSTRAINED_DIALOG_HEIGHT_THRESHOLD and isVerticallyConstrained:
const showModeSpacer =
constrainedHeight === undefined ||
constrainedHeight >= MIN_HEIGHT_WITH_MODE_SPACER;
const showFooterHint =
constrainedHeight === undefined ||
constrainedHeight >=
(showWorkspacePriorityWarning
? MIN_HEIGHT_WITH_WARNING_FOOTER_HINT
: MIN_HEIGHT_WITH_FOOTER_HINT);— qwen3.7-max via Qwen Code /review
| constrainedHeight === undefined | ||
| ? undefined | ||
| : constrainedHeight >= SHELL_CONFIRMATION_FIXED_ROWS + 2 | ||
| ? Math.max(2, constrainedHeight - SHELL_CONFIRMATION_FIXED_ROWS) |
There was a problem hiding this comment.
[Suggestion] The height-clamping idiom availableTerminalHeight === undefined ? undefined : Math.max(1, Math.floor(availableTerminalHeight)) is duplicated verbatim across ShellConfirmationDialog, ConsentPrompt, and ApprovalModeDialog. This PR centralizes dialog height constraints via getDialogMaxHeight() in layoutUtils.ts, but each dialog still re-implements this clamping step.
Consider extracting a shared utility:
export const clampDialogHeight = (h: number | undefined): number | undefined =>
h === undefined ? undefined : Math.max(1, Math.floor(h));— qwen3.7-max via Qwen Code /review
| <MaxSizedBox | ||
| maxHeight={commandPreviewHeight} | ||
| maxWidth={Math.max(1, contentWidth - 8)} | ||
| overflowDirection="top" |
There was a problem hiding this comment.
[Suggestion] In the constrained path, commands are rendered as plain <Text>{cmd}</Text>, while the unconstrained path uses <RenderInline text={cmd} /> which processes inline markdown. Commands will render differently depending on terminal height — markdown-like characters (backticks, asterisks) in shell commands will be formatted in one view but raw in the other.
| overflowDirection="top" | |
| <Text color={theme.text.link}> | |
| <RenderInline text={cmd} /> | |
| </Text> |
— qwen3.7-max via Qwen Code /review
| const promptHeight = | ||
| constrainedHeight === undefined | ||
| ? undefined | ||
| : Math.max(1, constrainedHeight - CONSENT_PROMPT_CHROME_ROWS); |
There was a problem hiding this comment.
[Suggestion] When constrainedHeight <= 8 (small terminals, tmux splits), promptHeight becomes 1 — the consent prompt text is clipped to a single row while the Yes/No RadioButtonSelect (with flexShrink={0}) remains fully visible and interactive. There is no truncation indicator, so users can approve a destructive operation without reading the warning context.
Consider adding a [Content truncated — resize terminal] indicator with flexShrink={0} when promptHeight <= 2, or requiring typed confirmation in very small terminals.
— qwen3.7-max via Qwen Code /review
| constrainedHeight === undefined | ||
| ? undefined | ||
| : constrainedHeight >= SHELL_CONFIRMATION_FIXED_ROWS + 2 | ||
| ? Math.max(2, constrainedHeight - SHELL_CONFIRMATION_FIXED_ROWS) |
There was a problem hiding this comment.
[Critical] When constrainedHeight < SHELL_CONFIRMATION_FIXED_ROWS + 2 (i.e. < 11), commandPreviewHeight is set to 0 and the ternary falls to the null branch — rendering no command list at all. The approve/deny radio options still appear with flexShrink={0}. The user is asked to approve shell commands they literally cannot see.
This triggers at terminal height ~13 with staticExtraHeight=3 (dialogMaxHeight = max(1, 13-3-2) = 8), which is common with tmux splits or small laptop windows. The existing test at line 137 confirms this is by design (expect(frame).not.toContain('cmd-10')).
The shell command confirmation — the most security-critical gate in the CLI — can be bypassed by simply having a small terminal. A user could approve rm -rf $HOME without ever seeing the command.
Consider one of:
- When
commandPreviewHeightwould be 0, drop the title/subtitle (they're already flexShrink-able) to reclaim rows for at least 1-2 lines of command preview. - Replace the
nullbranch with a visible indicator:"(N commands — resize terminal to preview)"so the user knows commands are hidden before approving. - Refuse to show approve options when commands are invisible — require the user to resize or press a key to reveal.
— qwen3.7-max via Qwen Code /review
|
Updated in 21994e9. Changes:
Local verification:
GitHub CI has restarted for the new head and is currently pending. |
| constrainedHeight !== undefined && | ||
| commandPreviewHeight === 0 && | ||
| commands.length > 0; | ||
| const compactHiddenCommandsLayout = |
There was a problem hiding this comment.
[Critical] compactHiddenCommandsLayout activates at constrainedHeight <= SHELL_CONFIRMATION_FIXED_ROWS (9), but the compact layout needs 6 content rows (title 1 + hidden-count message 1 + 4 radio options 4) while the content area is only constrainedHeight - border(2) - paddingY(0) = constrainedHeight - 2 rows. At constrainedHeight = 7: content area = 5, but 6 rows needed → the flexShrink={0} options Box overflows and overflow="hidden" clips the last option ("No (esc)").
The boundary is constrainedHeight >= 8 for the compact layout to fit (content area 6 = needed 6). This is reachable at realistic terminal sizes (e.g. terminalHeight 12 with staticExtraHeight 3 → getDialogMaxHeight(12, 3) = 7).
| const compactHiddenCommandsLayout = | |
| const compactHiddenCommandsLayout = | |
| commandsHidden && constrainedHeight <= SHELL_CONFIRMATION_FIXED_ROWS | |
| && constrainedHeight >= 8; |
Or add a further degradation tier that drops the hidden-count message when constrainedHeight < 8, leaving only title + 4 options (5 rows).
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 24431d9. For hidden commands below the 8-row approval threshold, the dialog now keeps the title and hidden-command warning visible but only exposes the cancel option. At 8 rows and above, the compact hidden-command layout can fit the warning plus all approval options. Added a 7-row regression test for this exact clipping case.
| onHighlight={handleModeHighlight} | ||
| isFocused={mode === 'mode'} | ||
| maxItemsToShow={10} | ||
| maxItemsToShow={maxModeItemsToShow} |
There was a problem hiding this comment.
[Suggestion] showScrollArrows is hard-coded to false. Before this PR, maxItemsToShow={10} always fit all 5 modes, so this was harmless. Now that maxModeItemsToShow is dynamic and can be less than modeItems.length at constrained heights, users see a truncated list with no visual indicator that more modes exist below the viewport.
| maxItemsToShow={maxModeItemsToShow} | |
| showScrollArrows={maxModeItemsToShow < modeItems.length} |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 24431d9. ApprovalModeDialog now derives showScrollArrows from the constrained mode-list row budget, includes the arrow rows in maxItemsToShow, and hides the non-warning footer hint when needed so the scroll state remains visible. Added coverage for the 8-row truncated list and the 10-row footer tradeoff.
|
Updated in 24431d9 after the latest review. Why there were still issues:
Changes in this update:
Local verification:
GitHub CI has restarted for the new head. |
| request={{ | ||
| commands: Array.from( | ||
| { length: 10 }, | ||
| (_, i) => `cmd-${String(i + 1).padStart(2, '0')}`, |
There was a problem hiding this comment.
[Suggestion] Boundary heights 9 and 11 are not exercised. Height 9 is the compactHiddenCommandsLayout threshold (subtitle and "Do you want to proceed?" disappear), and height 11 is where commandPreviewHeight transitions from 0 to 2 (commands become visible via MaxSizedBox). An off-by-one in SHELL_CONFIRMATION_FIXED_ROWS or MIN_HEIGHT_WITH_HIDDEN_COMMAND_OPTIONS would go undetected.
Consider adding test cases at these heights:
- Height 9: assert compact layout (no subtitle, no question text) with all four options still visible
- Height 11: assert commands are rendered inside
MaxSizedBox(command text visible, no "shell commands hidden" notice)
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in ef1ccc5. Added boundary coverage for height 9, where the compact hidden-command layout hides the subtitle/question while keeping all approval options visible, and height 11, where the dialog switches to the MaxSizedBox command preview instead of the hidden-command notice.
| availableTerminalHeight={8} | ||
| />, | ||
| ); | ||
|
|
There was a problem hiding this comment.
[Suggestion] The truncation notice threshold is availablePromptRows <= 2 (heights 7–9). This test covers height 8 (availablePromptRows=1) and height 12 (availablePromptRows=5), but the exact boundary is not pinned. Height 9 (availablePromptRows=2, last showing notice) and height 10 (availablePromptRows=3, first without notice) would catch an off-by-one in CONSENT_PROMPT_CHROME_ROWS or the <= 2 condition.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in ef1ccc5. Added boundary coverage for both sides of the consent prompt truncation threshold: height 9 keeps the truncation notice at the two-row prompt boundary, and height 10 verifies the first three-row prompt case without the notice.
|
All requested follow-ups have been addressed on the latest head The remaining |
tanzhenxin
left a comment
tanzhenxin
left a comment
There was a problem hiding this comment.
Review
The flicker fix is clean and well-targeted — seeding the scroll offset from the active index so a high initial selection no longer renders from the top and jumps — and it's backed by solid boundary-height tests. The shared height-reservation constant is a nice cleanup over the previous duplicated magic number, and the skill.ts change is a welcome bonus: the disabled-skill path was dropping the user's args, and this fixes it.
Approving to unblock this. There's one known limitation worth a follow-up, called out below so it doesn't get lost.
1. Shell commands can be approved while fully hidden in a constrained dialog (severity: medium · confidence: high)
When the dialog is squeezed into roughly 8 to 10 rows, the command preview collapses entirely — the user sees only "N shell commands hidden — resize terminal to review" — but the approve options (Yes, allow once, Always allow…) stay selectable, so a user can approve shell commands they never actually saw. The dialog already does the right thing below 8 rows, where it drops to cancel-only; the gap is that the "commands are hidden" condition keeps holding through 10 rows while the cancel-only restriction only kicks in below 8. It's reachable at non-extreme sizes — a modestly short terminal or a small split pane. Tying the cancel-only behavior to "are the commands hidden" rather than a separate row threshold would close it consistently with the existing below-8 handling. Not blocking the approval, but worth tightening in a follow-up since it's a safety-confirmation path.
Verdict
APPROVE — solid flicker fix and a bonus args correctness fix; one known limitation noted above for a follow-up.
3 participants
What this PR does
This PR constrains interactive selection and confirmation dialogs so they stay within the visible terminal frame when height constraints are active.
It also keeps actionable choices visible in small terminals by allowing long prompt or command content to shrink before the option list is clipped. Shell command confirmations now use an explicit hidden-lines indicator when the command preview is truncated, so users are not led to believe that all commands are visible.
Why it's needed
When a tall selection or confirmation dialog appears in a small terminal or tmux pane, the rendered frame can temporarily exceed the terminal height before footer measurement settles. Ink can respond by clearing and repainting the terminal, which shows up as an intermittent flicker.
The fix keeps dialog rendering bounded on the first frame while preserving the existing unconstrained-height mode.
Reviewer Test Plan
How to verify
Open the CLI in a small terminal or tmux pane, then trigger a selection or confirmation prompt with enough body content to exceed the visible rows. The dialog should appear without a full-screen flash, the selection choices should remain visible, and shell command confirmations should show a hidden-lines indicator when the command list is truncated.
Run the targeted regression tests:
cd packages/cli && npx vitest run src/ui/layouts/DefaultAppLayout.test.tsx src/ui/layouts/ScreenReaderAppLayout.test.tsx src/ui/components/ShellConfirmationDialog.test.tsx src/ui/components/ConsentPrompt.test.tsx src/ui/components/ApprovalModeDialog.test.tsx src/ui/components/shared/BaseSelectionList.test.tsxRun repository checks:
npm run lint,npm run typecheck,npm run build, andgit diff --check.Evidence (Before & After)
Before: the new layout regression test rendered a 22-line dialog frame in an 8-row terminal scenario, which exceeds the terminal height and can trigger a full terminal repaint.
After: the same scenario is capped to the terminal frame, shell and consent confirmations keep their choices visible under constrained height, truncated shell command previews show a hidden-lines indicator, and the targeted regression test suite passes with 52 passed and 1 skipped.
Tested on
Environment (optional)
Tested locally with the package-level Vitest command above plus repository lint, typecheck, build, and diff whitespace checks.
Risk & Scope
Linked Issues
N/A