fix(tui): Tighten message and tool spacing#4595
Open
chiga0 wants to merge 5 commits into
Open
Conversation
Contributor
📋 Review SummaryThis PR implements focused TUI spacing and density improvements by removing unnecessary blank rows between UI elements. The changes are well-scoped to three core components ( 🔍 General Feedback
🎯 Specific Feedback🟢 Medium
🔵 Low
✅ Highlights
|
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. |
Collaborator
Author
|
Added the 34s local visual verification GIF generated with
Artifacts:
|
wenshao
reviewed
May 28, 2026
|
|
||
| const output = lastFrame() ?? ''; | ||
| expect(output.startsWith('\n')).toBe(false); | ||
| expect(output).toContain('Read txt files'); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The two new tests confirm gemini and tool_use_summary types produce no leading spacer — but the inverse contract is untested. No test verifies that user / user_shell types still produce marginTop=1 (the leading spacer row). This is the highest-risk regression path: someone adding 'user' to the zero-margin switch cases in getHistoryItemMarginTop would silently remove the turn separator with no test failure.
Suggested change
| expect(output).toContain('Read txt files'); | |
| expect(output.startsWith('\n')).toBe(false); | |
| expect(output).toContain('Read txt files'); | |
| }); | |
| it('renders user prompts with a leading spacer row', () => { | |
| const item: HistoryItem = { | |
| id: 1, | |
| type: 'user', | |
| text: 'Hello', | |
| }; | |
| const { lastFrame } = renderWithProviders( | |
| <HistoryItemDisplay item={item} terminalWidth={100} isPending={false} />, | |
| ); | |
| const output = lastFrame() ?? ''; | |
| expect(output.startsWith('\n')).toBe(true); | |
| }); |
— qwen3.7-max via Qwen Code /review
- Set marginTop=0 for user and gemini message types to eliminate blank lines between Q&A turns and tool calls - Add half-block (▄) separator line above Composer input area, replacing the full blank row with a subtle blended color line - Add color-utils helpers: interpolateColor, supportsTrueColor - Add HalfLinePaddedBox component (reusable, currently used by Composer only) - Add design doc for TUI spacing density PR2 Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
requested changes
May 29, 2026
| <Box flexDirection="column" marginTop={1}> | ||
| <Box flexDirection="column" marginTop={separatorColor ? 0 : 1}> | ||
| {separatorColor && ( | ||
| <Text color={separatorColor}> |
Collaborator
There was a problem hiding this comment.
[Critical] The decorative ▄ separator renders unconditionally when truecolor is supported, without checking isScreenReaderEnabled. Screen reader users will hear a row of meaningless half-block characters announced as noise.
The hook is already imported at line 7 and used elsewhere in this file (e.g., <Footer /> gate). HalfLinePaddedBox in the same PR also correctly bails out for screen readers.
Suggested change
| <Text color={separatorColor}> | |
| {separatorColor && !isScreenReaderEnabled && ( |
— qwen3.7-max via Qwen Code /review
| * themes. The content area has no backgroundColor — it inherits the terminal's | ||
| * real background. | ||
| */ | ||
| export const HalfLinePaddedBox: React.FC<HalfLinePaddedBoxProps> = (props) => { |
Collaborator
There was a problem hiding this comment.
[Suggestion] HalfLinePaddedBox is exported but never imported anywhere in the codebase — it's dead code in this PR. The 89-line component adds maintenance burden without serving any current functionality.
Either remove it from this PR (re-introduce alongside its first consumer), or wire it into Composer.tsx to replace the inline separator logic that duplicates the same pattern (supportsTrueColor() + interpolateColor() + '▄'.repeat()).
— qwen3.7-max via Qwen Code /review
| color2: string, | ||
| factor: number, | ||
| ): string { | ||
| if (factor <= 0) { |
Collaborator
There was a problem hiding this comment.
[Suggestion] Two new exported functions (91 lines combined) have no test coverage. The existing color-utils.test.ts doesn't reference either function.
interpolateColor has untested branches: boundary factors (0, 1, negative, >1), mid-blend correctness, unparseable input returning '', 3-digit hex expansion, and Ink name lookup. supportsTrueColor has untested env-var paths and the getColorDepth() fallback.
Being pure functions, these are trivially testable with vi.stubEnv and standard assertions.
— qwen3.7-max via Qwen Code /review
Replace the interpolated purple band color with terminal-native dimColor rendering for ▄/▀ half-line characters. This avoids theme-dependent color mismatches while preserving the visual spacing effect. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
previously requested changes
May 29, 2026
| } | ||
|
|
||
| return ( | ||
| <Box flexDirection="column" width={width}> |
Collaborator
There was a problem hiding this comment.
[Critical] New a11y regression: the decorative ▄/▀ half-line separators render unconditionally whenever width is defined (which is always, via HistoryItemDisplay passing contentWidth). There is no useIsScreenReaderEnabled() guard, so screen-reader users will hear two rows of repeated block characters announced around every user message.
The project already has the right pattern in packages/cli/src/ui/components/shared/HalfLinePaddedBox.tsx:42-47 — it bails out with if (... || isScreenReaderEnabled) return <>{props.children}</>; and also handles the truecolor fallback and background-blended color. This component was introduced earlier in the same PR but is currently unused; reusing it here both fixes the regression and removes the dead-code concern from R2.
Suggested change
| <Box flexDirection="column" width={width}> | |
| export const UserMessage: React.FC<UserMessageProps> = ({ text, width }) => { | |
| const content = ( | |
| <PrefixedTextMessage | |
| text={text} | |
| prefix=">" | |
| prefixColor={theme.text.accent} | |
| textColor={theme.text.accent} | |
| ariaLabel={SCREEN_READER_USER_PREFIX} | |
| alignSelf="flex-start" | |
| /> | |
| ); | |
| if (width === undefined) { | |
| return content; | |
| } | |
| return ( | |
| <HalfLinePaddedBox bandColor={theme.text.accent} bandOpacity={0.15} width={width}> | |
| {content} | |
| </HalfLinePaddedBox> | |
| ); | |
| }; |
— qwen3.7-max via Qwen Code /review
| ariaLabel={SCREEN_READER_USER_PREFIX} | ||
| alignSelf="flex-start" | ||
| /> | ||
| ); |
Collaborator
There was a problem hiding this comment.
[Suggestion] '▄'.repeat(width) and '▀'.repeat(width) throw RangeError: Invalid count value when width is negative. width here is contentWidth = terminalWidth - 4 (HistoryItemDisplay.tsx:140), so during extremely narrow terminal resize (or transient init states before size detection), this becomes a hard unhandled exception. Previous code only fed contentWidth into ink <Box width> which clamps gracefully; this is the first path that hands it to String.prototype.repeat.
Suggested change
| ); | |
| export const UserMessage: React.FC<UserMessageProps> = ({ text, width }) => { | |
| const content = ( | |
| <PrefixedTextMessage | |
| text={text} | |
| prefix=">" | |
| prefixColor={theme.text.accent} | |
| textColor={theme.text.accent} | |
| ariaLabel={SCREEN_READER_USER_PREFIX} | |
| alignSelf="flex-start" | |
| /> | |
| ); | |
| if (width === undefined) { | |
| return content; | |
| } | |
| const safeWidth = Math.max(0, width); | |
| return ( | |
| <Box flexDirection="column" width={safeWidth}> | |
| <Text dimColor>{'▄'.repeat(safeWidth)}</Text> | |
| {content} | |
| <Text dimColor>{'▀'.repeat(safeWidth)}</Text> | |
| </Box> | |
| ); | |
| }; |
If the Critical above is addressed by switching to HalfLinePaddedBox, this guard belongs inside that component (it also calls repeat(width)) rather than here.
— qwen3.7-max via Qwen Code /review
| ); | ||
|
|
||
| const output = lastFrame() ?? ''; | ||
| expect(output.startsWith('\n')).toBe(false); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The two new tests cover the no-leading-spacer contract for gemini and tool_use_summary, but the incremental change in this PR — UserMessage now rendering ▄/▀ half-lines when width is provided — has no dedicated test coverage. The existing user-type tests (lines 37-59) only assert text content via toContain('Hello') and would pass even if the half-lines rendered incorrectly or not at all.
Worth adding:
width=positive→ half-lines rendered at correct count (expect(lastFrame()).toContain('▄'.repeat(76))withterminalWidth=80).width=undefined→ no half-lines rendered (negative assertion against▄/▀).- Screen-reader mode (
useIsScreenReaderEnabledmockedtrue) → half-lines suppressed. This test would currently fail, driving the fix for the Critical above. - Narrow terminal (
terminalWidth=3,contentWidth=-1) → no crash, text still rendered.
— qwen3.7-max via Qwen Code /review
wenshao
dismissed
their stale review
Dismissing — comments posted with line:null (Create Review API anchor failure). Re-posting individually via PR Comments API with side=RIGHT.
wenshao
reviewed
May 29, 2026
| } | ||
|
|
||
| return ( | ||
| <Box flexDirection="column" width={width}> |
Collaborator
There was a problem hiding this comment.
[Critical] New a11y regression: the decorative ▄/▀ half-line separators render unconditionally whenever width is defined (which is always, via HistoryItemDisplay passing contentWidth). There is no useIsScreenReaderEnabled() guard, so screen-reader users will hear two rows of repeated block characters announced around every user message.
The project already has the right pattern in packages/cli/src/ui/components/shared/HalfLinePaddedBox.tsx:42-47 — it bails out with if (... || isScreenReaderEnabled) return <>{props.children}</>; and also handles the truecolor fallback and background-blended color. This component was introduced earlier in the same PR but is currently unused; reusing it here both fixes the regression and removes the dead-code concern from R2.
Suggested change
| <Box flexDirection="column" width={width}> | |
| export const UserMessage: React.FC<UserMessageProps> = ({ text, width }) => { | |
| const content = ( | |
| <PrefixedTextMessage | |
| text={text} | |
| prefix=">" | |
| prefixColor={theme.text.accent} | |
| textColor={theme.text.accent} | |
| ariaLabel={SCREEN_READER_USER_PREFIX} | |
| alignSelf="flex-start" | |
| /> | |
| ); | |
| if (width === undefined) { | |
| return content; | |
| } | |
| return ( | |
| <HalfLinePaddedBox bandColor={theme.text.accent} bandOpacity={0.15} width={width}> | |
| {content} | |
| </HalfLinePaddedBox> | |
| ); | |
| }; |
— qwen3.7-max via Qwen Code /review
wenshao
reviewed
May 29, 2026
| ariaLabel={SCREEN_READER_USER_PREFIX} | ||
| alignSelf="flex-start" | ||
| /> | ||
| ); |
Collaborator
There was a problem hiding this comment.
[Suggestion] '▄'.repeat(width) and '▀'.repeat(width) throw RangeError: Invalid count value when width is negative. width here is contentWidth = terminalWidth - 4 (HistoryItemDisplay.tsx:140), so during extremely narrow terminal resize (or transient init states before size detection), this becomes a hard unhandled exception. Previous code only fed contentWidth into ink <Box width> which clamps gracefully; this is the first path that hands it to String.prototype.repeat.
Suggested change
| ); | |
| export const UserMessage: React.FC<UserMessageProps> = ({ text, width }) => { | |
| const content = ( | |
| <PrefixedTextMessage | |
| text={text} | |
| prefix=">" | |
| prefixColor={theme.text.accent} | |
| textColor={theme.text.accent} | |
| ariaLabel={SCREEN_READER_USER_PREFIX} | |
| alignSelf="flex-start" | |
| /> | |
| ); | |
| if (width === undefined) { | |
| return content; | |
| } | |
| const safeWidth = Math.max(0, width); | |
| return ( | |
| <Box flexDirection="column" width={safeWidth}> | |
| <Text dimColor>{'▄'.repeat(safeWidth)}</Text> | |
| {content} | |
| <Text dimColor>{'▀'.repeat(safeWidth)}</Text> | |
| </Box> | |
| ); | |
| }; |
If the Critical above is addressed by switching to HalfLinePaddedBox, this guard belongs inside that component (it also calls repeat(width)) rather than here.
— qwen3.7-max via Qwen Code /review
wenshao
reviewed
May 29, 2026
| ); | ||
|
|
||
| const output = lastFrame() ?? ''; | ||
| expect(output.startsWith('\n')).toBe(false); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The two new tests cover the no-leading-spacer contract for gemini and tool_use_summary, but the incremental change in this PR — UserMessage now rendering ▄/▀ half-lines when width is provided — has no dedicated test coverage. The existing user-type tests (lines 37-59) only assert text content via toContain('Hello') and would pass even if the half-lines rendered incorrectly or not at all.
Worth adding:
width=positive→ half-lines rendered at correct count (expect(lastFrame()).toContain('▄'.repeat(76))withterminalWidth=80).width=undefined→ no half-lines rendered (negative assertion against▄/▀).- Screen-reader mode (
useIsScreenReaderEnabledmockedtrue) → half-lines suppressed. This test would currently fail, driving the fix for the Critical above. - Narrow terminal (
terminalWidth=3,contentWidth=-1) → no crash, text still rendered.
— qwen3.7-max via Qwen Code /review
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
2 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.
What this PR does
This PR tightens TUI message and tool spacing without changing content, colors, borders, thinking visibility, SubAgent rendering, branding, theme behavior, confirmations, shell focus behavior, or compact-mode logic.
It centralizes normal conversation spacing so assistant continuations, status messages, tool summaries, and tool groups do not each add their own leading blank row. It also removes the extra blank row between a tool header/status line and its rendered result, and removes blank separator rows between adjacent expanded tool entries.
Why it's needed
Common Qwen Code sessions currently spend extra terminal rows on spacing before assistant output, between status/tool blocks, and inside expanded tool groups. That makes simple Q&A, file lists, shell output, errors, diffs, project inspection, and long streaming output harder to scan because users scroll through blank rows rather than useful content.
This is the first focused PR for QwenLM/qwen-code#4588. Keeping PR1 limited to spacing and density makes it independently reviewable before later PRs address thinking traces, SubAgent layout, borders, branding, or theme colors.
Reviewer Test Plan
How to verify
Run the CLI through the same simple Q&A and tool-output scenarios before and after this PR at a fixed terminal width. The expected behavior is that independent user turns still have one visual separator, assistant/tool/status follow-up blocks do not start with an additional spacer, tool result content starts directly below the tool header, and expanded multi-tool groups no longer show blank rows between adjacent tools.
Evidence (Before & After)
The terminal evidence renders the real Ink TUI components in a fixed 100-column fixture, captures both branches with tmux, and records a 34-second VHS GIF from the same fixture. No model call or network access is required for the fixture.
The existing snapshot coverage also records the same row-count reductions in the repository's current 80-column component fixtures.
Commands run
cd packages/cli && npx vitest run src/ui/components/HistoryItemDisplay.test.tsx src/ui/components/messages/ToolMessage.test.tsx src/ui/components/messages/ToolGroupMessage.test.tsx src/ui/utils/MarkdownDisplay.test.tsxnpm run build && npm run typecheckgit checkout origin/main && REPO_ROOT="$PWD" && /tmp/qwen-pr1-spacing-evidence/run-tmux-capture.sh "$REPO_ROOT" 'base origin/main 34b7d472e' basegit switch feat/tui-spacing-density-pr1 && REPO_ROOT="$PWD" && /tmp/qwen-pr1-spacing-evidence/run-tmux-capture.sh "$REPO_ROOT" 'PR1 fixed 848d6a166' fixedvhs /tmp/qwen-pr1-spacing-30s-1374RF/base.tapevhs /tmp/qwen-pr1-spacing-30s-1374RF/fixed.tapeTested on
Environment (optional)
Node.js v24.15.0. The build emitted pre-existing npm
always-auth, Browserslist, and VS Code companion curly-rule warnings, but completed successfully with 0 errors.Risk & Scope
Linked Issues
References QwenLM/qwen-code#4588.