-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(cli): improve rendering on narrow terminals #3968
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d175d88
0779de3
169031d
cada422
c6687db
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ | |
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { Box, useIsScreenReaderEnabled } from 'ink'; | ||
| import { Box, Text, useIsScreenReaderEnabled } from 'ink'; | ||
| import { useCallback, useState } from 'react'; | ||
| import { LoadingIndicator } from './LoadingIndicator.js'; | ||
| import { InputPrompt } from './InputPrompt.js'; | ||
|
|
@@ -15,6 +15,7 @@ import { useUIState } from '../contexts/UIStateContext.js'; | |
| import { useUIActions } from '../contexts/UIActionsContext.js'; | ||
| import { useVimMode } from '../contexts/VimModeContext.js'; | ||
| import { useConfig } from '../contexts/ConfigContext.js'; | ||
| import { theme } from '../semantic-colors.js'; | ||
| import { StreamingState, type HistoryItemToolGroup } from '../types.js'; | ||
| import { FeedbackDialog } from '../FeedbackDialog.js'; | ||
| import { t } from '../../i18n/index.js'; | ||
|
|
@@ -38,6 +39,13 @@ export const Composer = () => { | |
| const isStreaming = | ||
| uiState.streamingState === StreamingState.Responding || | ||
| uiState.streamingState === StreamingState.WaitingForConfirmation; | ||
| // `isStreaming` covers Responding|WaitingForConfirmation, but we only | ||
| // suppress during Responding (active token output). A confirmation prompt | ||
| // must remain visible regardless of width. Drop the redundant `isStreaming` | ||
| // guard so future expansions of `isStreaming` don't silently widen suppression. | ||
| const suppressBottomLoadingIndicator = | ||
| uiState.streamingState === StreamingState.Responding && | ||
| uiState.terminalWidth <= 30; | ||
|
|
||
| // Aggregate agent tool tokens from executing tool calls. Only changes when | ||
| // a subagent reports progress, so it doesn't drive the animation loop. | ||
|
|
@@ -80,7 +88,7 @@ export const Composer = () => { | |
|
|
||
| return ( | ||
| <Box flexDirection="column" marginTop={1}> | ||
| {!uiState.embeddedShellFocused && ( | ||
| {!uiState.embeddedShellFocused && !suppressBottomLoadingIndicator && ( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] This suppresses the whole Consider preserving a compact fallback such as — gpt-5.5 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch — adopted the minimal-fallback approach. Composer now renders a compact
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch — fixed in cada422. Switched to the existing |
||
| <LoadingIndicator | ||
| // Hide loading phrases when enableLoadingPhrases is explicitly false. | ||
| // Using === false ensures phrases show by default when undefined. | ||
|
|
@@ -102,6 +110,18 @@ export const Composer = () => { | |
| isReceivingContent={isReceivingContent} | ||
| /> | ||
| )} | ||
| {/* | ||
| * Narrow-terminal fallback: when the full LoadingIndicator is suppressed | ||
| * (≤30 cols, actively Responding) we still surface a minimal `esc to | ||
| * cancel` hint so users on ultra-narrow terminals retain the cancel | ||
| * affordance during long-running calls. The full timer/spinner/phrase | ||
| * UI is still suppressed to avoid layout breakage. | ||
| */} | ||
| {!uiState.embeddedShellFocused && suppressBottomLoadingIndicator && ( | ||
| <Box paddingLeft={2}> | ||
| <Text color={theme.text.secondary}>({t('Esc to cancel')})</Text> | ||
| </Box> | ||
| )} | ||
|
|
||
| <QueuedMessageDisplay messageQueue={uiState.messageQueue} /> | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -61,7 +61,7 @@ describe('<TableRenderer />', () => { | |||||||||||
| const output = renderTable( | ||||||||||||
| ['项目', 'ANSI', 'Markdown'], | ||||||||||||
| [['中文内容', '\u001b[31mRed\u001b[0m Blue', '**bold** and `code`']], | ||||||||||||
| 42, | ||||||||||||
| 80, | ||||||||||||
| ['left', 'center', 'right'], | ||||||||||||
| ); | ||||||||||||
| expectAllLinesToHaveSameVisibleWidth(output); | ||||||||||||
|
|
@@ -110,19 +110,27 @@ describe('<TableRenderer />', () => { | |||||||||||
| expect(output).toContain('wrap'); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| // Alignment tests use contentWidth ≥ 60 so horizontal mode is exercised | ||||||||||||
| // (vertical mode renders key:value pairs and bypasses pad alignment). | ||||||||||||
|
|
||||||||||||
| it('respects left alignment', () => { | ||||||||||||
| const output = renderTable(['Header'], [['left']], 30, ['left']); | ||||||||||||
| const output = renderTable(['Header'], [['left']], 60, ['left']); | ||||||||||||
| expect(output).toContain('left'); | ||||||||||||
| // Horizontal-mode guard so this test fails loudly if the threshold | ||||||||||||
| // is bumped back above 60 and the test silently degrades to vertical. | ||||||||||||
| expect(output).toContain('┌'); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| it('respects center alignment', () => { | ||||||||||||
| const output = renderTable(['Header'], [['center']], 30, ['center']); | ||||||||||||
| const output = renderTable(['Header'], [['center']], 60, ['center']); | ||||||||||||
| expect(output).toContain('center'); | ||||||||||||
| expect(output).toContain('┌'); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| it('respects right alignment', () => { | ||||||||||||
| const output = renderTable(['Header'], [['right']], 30, ['right']); | ||||||||||||
| const output = renderTable(['Header'], [['right']], 60, ['right']); | ||||||||||||
| expect(output).toContain('right'); | ||||||||||||
| expect(output).toContain('┌'); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| it('handles multiple columns with mixed alignment', () => { | ||||||||||||
|
|
@@ -458,6 +466,89 @@ describe('<TableRenderer />', () => { | |||||||||||
| expect(output).toContain('很长的值一'); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| // ─── Narrow-terminal vertical fallback ─── | ||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The tests cover widths below and above the new strict Consider adding boundary assertions that exactly — gpt-5.5 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adopted. Added equality boundary tests at |
||||||||||||
| describe('horizontal/vertical mode threshold', () => { | ||||||||||||
| it('uses horizontal mode at ample width (60 cols, 2 short cols)', () => { | ||||||||||||
| const output = renderTable(['A', 'B'], [['x', 'y']], 60); | ||||||||||||
| // Horizontal markers must be present. | ||||||||||||
| expect(output).toContain('┌'); | ||||||||||||
| expect(output).toContain('└'); | ||||||||||||
| expect(output).toContain('│'); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| it('falls back to vertical below the absolute floor (≤24 cols)', () => { | ||||||||||||
| // ABSOLUTE_MIN_HORIZONTAL_TABLE_WIDTH is 24. | ||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] 缺少
Suggested change
— deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adopted. Added the 2-col equality test at |
||||||||||||
| const output = renderTable(['A', 'B'], [['x', 'y']], 20); | ||||||||||||
| // No horizontal table border characters in vertical mode. | ||||||||||||
| expect(output).not.toContain('┌'); | ||||||||||||
| expect(output).not.toContain('└'); | ||||||||||||
| // Vertical mode renders "label:" pairs. | ||||||||||||
| expect(output).toContain('A:'); | ||||||||||||
| expect(output).toContain('B:'); | ||||||||||||
| expect(output).toContain('x'); | ||||||||||||
| expect(output).toContain('y'); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| it('promotes to horizontal once column-budget threshold is met (2 cols, ~30 cols)', () => { | ||||||||||||
| // borderOverhead = 1 + 2*3 = 7; minHorizontal = max(24, 2*3 + 7 + 4) = 24 | ||||||||||||
| // so 30 cols comfortably fits horizontal. | ||||||||||||
| const output = renderTable(['A', 'B'], [['x', 'y']], 30); | ||||||||||||
| expect(output).toContain('┌'); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| // Boundary equality tests: the comparator is strict `<`, so the threshold | ||||||||||||
| // value itself must still render horizontally. Without these, a future | ||||||||||||
| // off-by-one change from `<` to `<=` would slip through the < / > pair. | ||||||||||||
| it('renders horizontal at exact absolute floor (2 cols, contentWidth=24)', () => { | ||||||||||||
| // ABSOLUTE_MIN_HORIZONTAL_TABLE_WIDTH is 24. With strict `<`, equality | ||||||||||||
| // means horizontal mode is selected. | ||||||||||||
| const output = renderTable(['A', 'B'], [['x', 'y']], 24); | ||||||||||||
| expect(output).toContain('┌'); | ||||||||||||
| expect(output).toContain('└'); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| it('falls back to vertical one below absolute floor (2 cols, contentWidth=23)', () => { | ||||||||||||
| const output = renderTable(['A', 'B'], [['x', 'y']], 23); | ||||||||||||
| expect(output).not.toContain('┌'); | ||||||||||||
| expect(output).toContain('A:'); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| it('renders horizontal at exact column-budget threshold (5 cols, contentWidth=35)', () => { | ||||||||||||
| // 5 cols → minHorizontal = 5*3 + (1+5*3) + 4 = 35. Equality must still | ||||||||||||
| // render horizontally under the strict `<` comparator. | ||||||||||||
| const output = renderTable( | ||||||||||||
| ['A', 'B', 'C', 'D', 'E'], | ||||||||||||
| [['1', '2', '3', '4', '5']], | ||||||||||||
| 35, | ||||||||||||
| ); | ||||||||||||
| expect(output).toContain('┌'); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| it('falls back to vertical one below column-budget threshold (5 cols, contentWidth=34)', () => { | ||||||||||||
| const output = renderTable( | ||||||||||||
| ['A', 'B', 'C', 'D', 'E'], | ||||||||||||
| [['1', '2', '3', '4', '5']], | ||||||||||||
| 34, | ||||||||||||
| ); | ||||||||||||
| expect(output).not.toContain('┌'); | ||||||||||||
| expect(output).toContain('A:'); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| it('forces vertical for many-column tables on narrow terminals', () => { | ||||||||||||
| // 5 cols → minHorizontal = 5*3 + (1+5*3) + 4 = 35; 30 cols is below that. | ||||||||||||
| const output = renderTable( | ||||||||||||
| ['A', 'B', 'C', 'D', 'E'], | ||||||||||||
| [['1', '2', '3', '4', '5']], | ||||||||||||
| 30, | ||||||||||||
| ); | ||||||||||||
| expect(output).not.toContain('┌'); | ||||||||||||
| // Should still surface the data. | ||||||||||||
| expect(output).toContain('A:'); | ||||||||||||
| expect(output).toContain('1'); | ||||||||||||
| expect(output).toContain('5'); | ||||||||||||
| }); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| it('stays stable across multiple content widths', () => { | ||||||||||||
| for (const width of [8, 10, 12, 16, 20, 30, 40, 60]) { | ||||||||||||
| const output = renderTable( | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -18,6 +18,13 @@ const MIN_COLUMN_WIDTH = 3; | |||||||||||||||||||
| /** Maximum number of lines per row before switching to vertical format */ | ||||||||||||||||||||
| const MAX_ROW_LINES = 4; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** | ||||||||||||||||||||
| * Below this width the column-aware budget (see `minHorizontalTableWidth` | ||||||||||||||||||||
| * below) is bypassed and we always switch to vertical: even a 1-column | ||||||||||||||||||||
| * table is barely readable horizontally under ~24 cols of content. | ||||||||||||||||||||
| */ | ||||||||||||||||||||
| const ABSOLUTE_MIN_HORIZONTAL_TABLE_WIDTH = 24; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** Safety margin to account for terminal resize races */ | ||||||||||||||||||||
| const SAFETY_MARGIN = 4; | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -324,7 +331,10 @@ export const TableRenderer: React.FC<TableRendererProps> = ({ | |||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // ── Step 2: Calculate available space ── | ||||||||||||||||||||
| // Border overhead: │ content │ content │ = 1 + (width + 3) per column | ||||||||||||||||||||
| // Border overhead: │ content │ content │ = 1 + (width + 3) per column. | ||||||||||||||||||||
| // NOTE: this value is reused below in the horizontal-vs-vertical threshold | ||||||||||||||||||||
| // (`minHorizontalTableWidth`). Any change to this formula will silently | ||||||||||||||||||||
| // shift the layout threshold — adjust both call sites together. | ||||||||||||||||||||
| const borderOverhead = 1 + colCount * 3; | ||||||||||||||||||||
| const availableWidth = Math.max( | ||||||||||||||||||||
| contentWidth - borderOverhead - SAFETY_MARGIN, | ||||||||||||||||||||
|
|
@@ -392,7 +402,18 @@ export const TableRenderer: React.FC<TableRendererProps> = ({ | |||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const maxRowLines = calculateMaxRowLines(); | ||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Consider hoisting the threshold check before column-width allocation:
Suggested change
— deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adopted in 7f77d8d. Hoisted the |
||||||||||||||||||||
| const useVerticalFormat = maxRowLines > MAX_ROW_LINES; | ||||||||||||||||||||
| // Column-aware horizontal-vs-vertical decision: a horizontal table needs | ||||||||||||||||||||
| // at least `MIN_COLUMN_WIDTH` per column plus the border overhead computed | ||||||||||||||||||||
| // above, with a safety margin. This avoids the prior fixed 60-col floor | ||||||||||||||||||||
| // that forced vertical mode for a 2-col table on a 50-col terminal even | ||||||||||||||||||||
| // when content fit comfortably. The downstream `maxLineWidth` safety | ||||||||||||||||||||
| // check still catches content that would actually overflow. | ||||||||||||||||||||
| const minHorizontalTableWidth = Math.max( | ||||||||||||||||||||
| ABSOLUTE_MIN_HORIZONTAL_TABLE_WIDTH, | ||||||||||||||||||||
| colCount * MIN_COLUMN_WIDTH + borderOverhead + SAFETY_MARGIN, | ||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion]
Suggested change
— deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adopted via expanded comment rather than a new constant. The current usage isn't a separate magic threshold — it really is the same border overhead being added to the column-width budget — so introducing |
||||||||||||||||||||
| ); | ||||||||||||||||||||
| const useVerticalFormat = | ||||||||||||||||||||
| contentWidth < minHorizontalTableWidth || maxRowLines > MAX_ROW_LINES; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // ── Helper: Get alignment for a column ── | ||||||||||||||||||||
| const getAlign = (colIndex: number): ColumnAlign => | ||||||||||||||||||||
|
|
||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] Magic number
30— the narrow-terminal suppression threshold is a bare literal with no named constant or rationale comment.LoadingIndicatorindependently uses 80 for its internalisNarrowWidth()threshold. If LoadingIndicator is later restyled and grows wider than 30 columns, the suppression threshold silently becomes too aggressive.— deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adopted in 7f77d8d. Extracted to
SUPPRESS_LOADING_INDICATOR_MAX_WIDTHwith a JSDoc that flags the relationship toLoadingIndicator's independent internalisNarrowWidth()threshold (80) so a future LoadingIndicator restyle has a single, documented knob to adjust.