fix(cli): improve rendering on narrow terminals#3968
Merged
Conversation
- TableRenderer: switch to vertical format when contentWidth < 60 cols, preventing wide horizontal tables from overflowing into scrollback on narrow terminals. - Composer: suppress bottom loading indicator when terminal width ≤ 30 cols during streaming, avoiding unnecessary redraws on ultra-narrow terminals. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
6 tasks
wenshao
reviewed
May 8, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
测试覆盖建议
1. 缺少 suppressBottomLoadingIndicator 的测试 — 新增的三条件分支（terminalWidth ≤ 30 + Responding）没有单元测试。建议添加：
terminalWidth: 30+Responding→ 隐藏 LoadingIndicatorterminalWidth: 31+Responding→ 显示 LoadingIndicatorterminalWidth: 25+WaitingForConfirmation→ 显示 LoadingIndicator
2. 缺少 MIN_HORIZONTAL_TABLE_WIDTH < 60 触发垂直格式的测试 — 建议添加 renderTable 在 contentWidth: 58 vs 60 时的格式断言（检查水平边框 ┌ 的存在性）。
3. 现有对齐测试（左/中/右）在垂直模式下变为空操作 — 三个使用 contentWidth: 30 的对齐测试现均在垂直模式渲染，不再验证对齐行为。建议提升 contentWidth 至 ≥60 或添加水平模式守卫（output.includes('┌')）。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| uiState.streamingState === StreamingState.Responding || | ||
| uiState.streamingState === StreamingState.WaitingForConfirmation; | ||
| const suppressBottomLoadingIndicator = | ||
| isStreaming && |
Collaborator
There was a problem hiding this comment.
[Suggestion] isStreaming && 在此处是冗余条件。isStreaming 定义为 Responding || WaitingForConfirmation，而后续又用 === Responding 收窄，因此 isStreaming && === Responding 等价于 === Responding。冗余增加了维护风险：若未来有人扩展 isStreaming 的定义，会误以为此 guard 已覆盖新状态。
Suggested change
| isStreaming && | |
| const suppressBottomLoadingIndicator = | |
| uiState.streamingState === StreamingState.Responding && | |
| uiState.terminalWidth <= 30; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Good catch — accepted in 0779de3. Dropped the redundant isStreaming && guard. Kept the explicit === Responding check so any future expansion of isStreaming (e.g. a new Idle-ish substate) doesn't silently widen suppression. Added a test pinning that WaitingForConfirmation on a 25-col terminal still shows the indicator.
| /** Maximum number of lines per row before switching to vertical format */ | ||
| const MAX_ROW_LINES = 4; | ||
|
|
||
| /** Narrow terminals make horizontal tables unreadable and scrollback-heavy. */ |
Collaborator
There was a problem hiding this comment.
[Suggestion] MIN_HORIZONTAL_TABLE_WIDTH = 60 是内容无关的硬阈值。一个 2 列、值为短字符串的表格在 50 列宽度下完全可以水平渲染，但会被强制切为垂直格式。渲染管线下游已有 maxLineWidth > contentWidth - SAFETY_MARGIN 的内容感知安全检查作为兜底，但新阈值直接绕过了它。建议：降低阈值至 40，或改为基于实际列宽的动态判断（如 colCount * MIN_COLUMN_WIDTH + borderOverhead + SAFETY_MARGIN）。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fair point — the fixed 60-col floor was content-agnostic. Replaced in 0779de3 with a column-aware threshold:
const minHorizontalTableWidth = Math.max(
ABSOLUTE_MIN_HORIZONTAL_TABLE_WIDTH, // 24, the readability floor
colCount * MIN_COLUMN_WIDTH + borderOverhead + SAFETY_MARGIN,
);So a 2-col table with short values now renders horizontally on a ~30-col terminal (2*3 + 7 + 4 = 17 < 30), while a 5-col table on the same width correctly falls back to vertical (5*3 + 16 + 4 = 35 > 30). The downstream maxLineWidth > contentWidth - SAFETY_MARGIN check is still in place as the content-aware safety net you noted. Added explicit threshold tests covering both directions.
Address review feedback on #3968: - Composer: drop redundant `isStreaming &&` guard from `suppressBottomLoadingIndicator`; the trailing `=== StreamingState.Responding` already implies streaming, and the redundancy risked future drift if `isStreaming` were extended. - Composer.test: add four cases pinning the suppression contract — Responding @ 25/30 cols hides, @ 31 cols shows, and WaitingForConfirmation @ 25 cols still shows so confirmation prompts never disappear on narrow terminals. - TableRenderer: replace the content-agnostic 60-col floor with a column-aware threshold (`max(24, colCount * MIN_COLUMN_WIDTH + borderOverhead + SAFETY_MARGIN)`) so a 2-column table with short values renders horizontally on a ~30-col terminal instead of being forced into vertical mode. The existing `maxLineWidth` post-build check still catches actual overflow. - TableRenderer.test: add explicit horizontal-vs-vertical threshold cases (2 cols @ 60/30/20 and 5 cols @ 30) and bump the alignment tests to contentWidth=60 with `┌` guards so they fail loudly if the threshold ever pushes them back into vertical no-op mode. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Author
|
Thanks for the test-coverage review @wenshao — addressed all three suggestions in 0779de3: 1.
2. 3. Alignment tests degrading to vertical no-ops — bumped the three left/center/right alignment tests from Full vitest suite for |
wenshao
reviewed
May 9, 2026
| return ( | ||
| <Box flexDirection="column" marginTop={1}> | ||
| {!uiState.embeddedShellFocused && ( | ||
| {!uiState.embeddedShellFocused && !suppressBottomLoadingIndicator && ( |
Collaborator
There was a problem hiding this comment.
[Suggestion] This suppresses the whole LoadingIndicator on ultra-narrow terminals while responding. That component also carries the visible esc to cancel affordance, so this removes the only on-screen cancel hint during the active long-running state.
Consider preserving a compact fallback such as esc to cancel, or suppressing only the animation/phrase/timer-heavy parts while keeping the cancel hint visible.
— gpt-5.5 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Good catch — adopted the minimal-fallback approach. Composer now renders a compact (esc to cancel) text line at paddingLeft={2} whenever the full LoadingIndicator is suppressed (Responding on ≤30 cols). The spinner/phrase/timer remains hidden to avoid layout breakage on a 25-col terminal, but the cancel affordance stays visible. Added two Composer tests asserting the fallback shows when (and only when) the indicator is suppressed. Fixed in 169031d.
Collaborator
Author
There was a problem hiding this comment.
Good catch — fixed in cada422. Switched to the existing t('Esc to cancel') key (option 1) and moved the parentheses outside the t() call so they are layout-only, not translatable. All 9 locales now resolve correctly.
| expect(output).toContain('很长的值一'); | ||
| }); | ||
|
|
||
| // ─── Narrow-terminal vertical fallback ─── |
Collaborator
There was a problem hiding this comment.
[Suggestion] The tests cover widths below and above the new strict contentWidth < minHorizontalTableWidth threshold, but not equality. A future off-by-one regression from < to <= would force vertical rendering at the documented threshold while these tests still pass.
Consider adding boundary assertions that exactly 24 for two short columns and exactly 35 for five short columns still render horizontally, optionally paired with 23/34 vertical assertions.
— gpt-5.5 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Adopted. Added equality boundary tests at contentWidth=24 (2-col absolute floor — ABSOLUTE_MIN_HORIZONTAL_TABLE_WIDTH) and contentWidth=35 (5-col column-budget threshold), paired with 23/34 vertical assertions. With the strict < comparator equality must still render horizontally, so a future < → <= flip is now caught. Fixed in 169031d.
wenshao
requested changes
May 9, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Composer.tsx:90：LoadingIndicator 在 ≤30 列 Responding 时完全消失 → 零用户反馈（已在 @wenshao 的开放评论中讨论，本审查将其升级为 Critical）。
[Suggestion] AgentComposer.tsx:265 — AgentComposer 缺少窄终端 LoadingIndicator 抑制逻辑，与主 Composer 不一致。在 ≤30 列终端上 AgentComposer 的 LoadingIndicator 可能溢出窗格宽度。建议提取共享的窄终端处理 hook。
| // 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.
[Suggestion] borderOverhead（第 335 行定义为 1 + colCount * 3）在 75 行后被复用于语义不同的目的（格式阈值 vs 可用宽度计算）。仅有一条散文注释连接两者，未直接命名变量。对 borderOverhead 看似无害的样式更改会静默改变水平/垂直格式决策阈值。
Suggested change
| colCount * MIN_COLUMN_WIDTH + borderOverhead + SAFETY_MARGIN, | |
| // borderOverhead is also used by minHorizontalTableWidth below for format selection. | |
| const borderOverhead = 1 + colCount * 3; |
— deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
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 MIN_HORIZONTAL_TABLE_BORDER_OVERHEAD would just alias the existing name. Instead I expanded the comment at the borderOverhead definition to flag the dual usage and require updating both call sites together. Fixed in 169031d.
| }); | ||
|
|
||
| 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.
[Suggestion] 缺少 ABSOLUTE_MIN_HORIZONTAL_TABLE_WIDTH（24）处的相等边界测试。当前测试覆盖了 contentWidth=20（低于）和 contentWidth=30（高于），但跳过了 contentWidth=24（等于）。如果 < 意外改为 <=，此边界是唯一能捕获的地方。同样缺少 5 列表在 contentWidth=35（列预算阈值）的相等测试。
Suggested change
| // ABSOLUTE_MIN_HORIZONTAL_TABLE_WIDTH is 24. | |
| it('uses horizontal at absolute floor boundary (2 cols, 24 cols)', () => { | |
| const output = renderTable(['A', 'B'], [['x', 'y']], 24); | |
| expect(output).toContain('┌'); | |
| }); |
— deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Adopted. Added the 2-col equality test at contentWidth=24 plus the 5-col equality test at contentWidth=35 (the column-budget threshold), each paired with one-below vertical assertions (23 and 34). All four guard the strict < comparator. Fixed in 169031d.
Address review feedback on the narrow-terminal rendering changes: - Composer: when the full LoadingIndicator is suppressed on ≤30-col terminals during Responding, render a minimal "(esc to cancel)" text fallback so users retain the cancel affordance. Suppressing the full indicator still avoids layout breakage, but the affordance now stays visible. - TableRenderer: clarify that `borderOverhead` is reused by the horizontal-vs-vertical layout threshold so a future change to the border-width formula does not silently shift the threshold. - TableRenderer tests: add equality boundary cases at `ABSOLUTE_MIN_HORIZONTAL_TABLE_WIDTH` (24) and at the 5-column column-budget threshold (35), plus one-below cases, so a future `<` → `<=` regression on the strict comparator is caught. - Composer tests: assert the fallback string is rendered when (and only when) the full indicator is suppressed. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Author
|
All @wenshao's review feedback addressed in 169031d: esc-to-cancel preserved via compact fallback at paddingLeft=2, borderOverhead dual-usage clarified in JSDoc (rename declined — the reuse is the same border overhead in both contexts), equality boundary tests added for both 2-col (contentWidth=24) and 5-col (contentWidth=35) thresholds. 71/71 tests passing in Composer + TableRenderer, CI all green. Ready for re-review. |
wenshao
requested changes
May 11, 2026
| */} | ||
| {!uiState.embeddedShellFocused && suppressBottomLoadingIndicator && ( | ||
| <Box paddingLeft={2}> | ||
| <Text color={theme.text.secondary}>{t('(esc to cancel)')}</Text> |
Collaborator
There was a problem hiding this comment.
[Critical] t('(esc to cancel)') 在所有 locale 文件中不存在该翻译键（已验证 en/zh/ja/fr/de/pt/ru/ca/zh-TW）。t() 遇到未知键会回退返回键本身，因此英文用户看到正确文本，但非英文 locale 用户将看到未翻译的英文 (esc to cancel)。
项目已有翻译好的键 'Esc to cancel'（在全部 9 个 locale 文件中，QwenOAuthProgress.tsx 等多处使用）。
建议修复（推荐方案 1）：
Suggested change
| <Text color={theme.text.secondary}>{t('(esc to cancel)')}</Text> | |
| <Text color={theme.text.secondary}>({t('Esc to cancel')})</Text> |
备选方案 2：
Suggested change
| <Text color={theme.text.secondary}>{t('(esc to cancel)')}</Text> | |
| <Text color={theme.text.secondary}>{t('Esc to cancel')}</Text> |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
added 2 commits
May 11, 2026 15:20
…allback The 169031d fix passed `'(esc to cancel)'` to `t()` but no such key exists in any locale file. `t()` falls back to returning the key verbatim, so English users saw correct text but non-English locales got the untranslated English string — i.e. zero i18n coverage. The repo already ships a translated `'Esc to cancel'` key in all 9 locales (used by QwenOAuthProgress and similar). Reuse it and move the parentheses outside the call so the surrounding `()` is layout-only, not translatable.
The cada422 fix switched the fallback from `t('(esc to cancel)')` to `({t('Esc to cancel')})` (using the existing translated key). The Composer tests still asserted lowercase 'esc to cancel' which no longer appears in the rendered output. Bump the two assertions to match the new casing.
wenshao
approved these changes
May 11, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
[Suggestion] packages/cli/src/ui/utils/TableRenderer.test.tsx:277 — The padAligned: center alignment with odd padding test uses contentWidth=10 which now silently falls into vertical mode (10 < ABSOLUTE_MIN_HORIZONTAL_TABLE_WIDTH = 24). padAligned is never invoked, but the test still passes because expect(output).toContain('A') is trivially satisfied in vertical key:value output. The test no longer verifies alignment behavior.
Consider bumping contentWidth to 60 (matching the other alignment tests).
— deepseek-v4-pro via Qwen Code /review
| // guard so future expansions of `isStreaming` don't silently widen suppression. | ||
| const suppressBottomLoadingIndicator = | ||
| uiState.streamingState === StreamingState.Responding && | ||
| uiState.terminalWidth <= 30; |
Collaborator
There was a problem hiding this comment.
[Suggestion] Magic number 30 — the narrow-terminal suppression threshold is a bare literal with no named constant or rationale comment. LoadingIndicator independently uses 80 for its internal isNarrowWidth() threshold. If LoadingIndicator is later restyled and grows wider than 30 columns, the suppression threshold silently becomes too aggressive.
Suggested change
| uiState.terminalWidth <= 30; | |
| const SUPPRESS_LOADING_INDICATOR_MAX_WIDTH = 30; | |
| const suppressBottomLoadingIndicator = | |
| uiState.streamingState === StreamingState.Responding && | |
| uiState.terminalWidth <= SUPPRESS_LOADING_INDICATOR_MAX_WIDTH; |
— deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Adopted in 7f77d8d. Extracted to SUPPRESS_LOADING_INDICATOR_MAX_WIDTH with a JSDoc that flags the relationship to LoadingIndicator's independent internal isNarrowWidth() threshold (80) so a future LoadingIndicator restyle has a single, documented knob to adjust.
| @@ -392,7 +402,18 @@ export const TableRenderer: React.FC<TableRendererProps> = ({ | |||
| } | |||
|
|
|||
| const maxRowLines = calculateMaxRowLines(); | |||
Collaborator
There was a problem hiding this comment.
[Suggestion] calculateMaxRowLines() (iterates every cell through wrapText) runs before the minHorizontalTableWidth check. When contentWidth < minHorizontalTableWidth triggers vertical mode, all prior column-width allocation and wrapping work is discarded. The new column-aware threshold intends to provide a fast-path but doesn't actually short-circuit the expensive computation.
Consider hoisting the threshold check before column-width allocation:
Suggested change
| const maxRowLines = calculateMaxRowLines(); | |
| const minHorizontalTableWidth = Math.max( | |
| ABSOLUTE_MIN_HORIZONTAL_TABLE_WIDTH, | |
| colCount * MIN_COLUMN_WIDTH + borderOverhead + SAFETY_MARGIN, | |
| ); | |
| if (contentWidth < minHorizontalTableWidth) { | |
| return <Box marginY={1}><Text>{renderVerticalFormat()}</Text></Box>; | |
| } | |
| // Only compute columnWidths and maxRowLines for the horizontal path |
— deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Adopted in 7f77d8d. Hoisted the minHorizontalTableWidth check above calculateMaxRowLines() and early-return to the vertical format. The per-cell wrapText pass (and the column-width allocation right before it) is now skipped whenever the terminal is already too narrow for horizontal layout — that work was discarded on the prior code path. Behavior is unchanged: same threshold, same vertical fallback, same secondary MAX_ROW_LINES check still gates the horizontal path. All 71 tests in Composer.test.tsx + TableRenderer.test.tsx pass; the existing equality-boundary tests at contentWidth 24/34/35 continue to pin the strict < comparator.
8 tasks
wenshao
pushed a commit
that referenced
this pull request
May 17, 2026
* fix(cli): improve rendering on narrow terminals - TableRenderer: switch to vertical format when contentWidth < 60 cols, preventing wide horizontal tables from overflowing into scrollback on narrow terminals. - Composer: suppress bottom loading indicator when terminal width ≤ 30 cols during streaming, avoiding unnecessary redraws on ultra-narrow terminals. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(cli): cover narrow-terminal rendering branches + tighten thresholds Address review feedback on #3968: - Composer: drop redundant `isStreaming &&` guard from `suppressBottomLoadingIndicator`; the trailing `=== StreamingState.Responding` already implies streaming, and the redundancy risked future drift if `isStreaming` were extended. - Composer.test: add four cases pinning the suppression contract — Responding @ 25/30 cols hides, @ 31 cols shows, and WaitingForConfirmation @ 25 cols still shows so confirmation prompts never disappear on narrow terminals. - TableRenderer: replace the content-agnostic 60-col floor with a column-aware threshold (`max(24, colCount * MIN_COLUMN_WIDTH + borderOverhead + SAFETY_MARGIN)`) so a 2-column table with short values renders horizontally on a ~30-col terminal instead of being forced into vertical mode. The existing `maxLineWidth` post-build check still catches actual overflow. - TableRenderer.test: add explicit horizontal-vs-vertical threshold cases (2 cols @ 60/30/20 and 5 cols @ 30) and bump the alignment tests to contentWidth=60 with `┌` guards so they fail loudly if the threshold ever pushes them back into vertical no-op mode. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): preserve esc-to-cancel on narrow terminals + boundary tests Address review feedback on the narrow-terminal rendering changes: - Composer: when the full LoadingIndicator is suppressed on ≤30-col terminals during Responding, render a minimal "(esc to cancel)" text fallback so users retain the cancel affordance. Suppressing the full indicator still avoids layout breakage, but the affordance now stays visible. - TableRenderer: clarify that `borderOverhead` is reused by the horizontal-vs-vertical layout threshold so a future change to the border-width formula does not silently shift the threshold. - TableRenderer tests: add equality boundary cases at `ABSOLUTE_MIN_HORIZONTAL_TABLE_WIDTH` (24) and at the 5-column column-budget threshold (35), plus one-below cases, so a future `<` → `<=` regression on the strict comparator is caught. - Composer tests: assert the fallback string is rendered when (and only when) the full indicator is suppressed. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): use existing 'Esc to cancel' i18n key for narrow-terminal fallback The 169031d fix passed `'(esc to cancel)'` to `t()` but no such key exists in any locale file. `t()` falls back to returning the key verbatim, so English users saw correct text but non-English locales got the untranslated English string — i.e. zero i18n coverage. The repo already ships a translated `'Esc to cancel'` key in all 9 locales (used by QwenOAuthProgress and similar). Reuse it and move the parentheses outside the call so the surrounding `()` is layout-only, not translatable. * test(Composer): update esc-fallback assertions to match i18n key casing The cada422 fix switched the fallback from `t('(esc to cancel)')` to `({t('Esc to cancel')})` (using the existing translated key). The Composer tests still asserted lowercase 'esc to cancel' which no longer appears in the rendered output. Bump the two assertions to match the new casing. --------- Co-authored-by: 秦奇 <gary.gq@alibaba-inc.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two narrow-terminal rendering fixes extracted from the TUI flicker hardening work:
contentWidth < 60columns, preventing wide horizontal tables from overflowing into scrollback on narrow terminals. Previously onlymaxRowLines > 4triggered vertical format.StreamingState.Responding), avoiding unnecessary redraws on ultra-narrow terminals.Test plan
npx vitest run packages/cli/src/ui/utils/TableRenderer.test.tsx— 43 tests passnpx vitest run packages/cli/src/ui/components/Composer.test.tsx— 14 tests pass🤖 Generated with Qwen Code