Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import { Box, Text } from 'ink';
import type { IndividualToolCallDisplay } from '../../types.js';
import { ToolCallStatus } from '../../types.js';
import type { AnsiOutputDisplay } from '@qwen-code/qwen-code-core';
import { SHELL_COMMAND_NAME, SHELL_NAME } from '../../constants.js';
import { theme } from '../../semantic-colors.js';
import { localizeToolDisplayName, t } from '../../../i18n/index.js';
import { ToolStatusIndicator } from '../shared/ToolStatusIndicator.js';
Expand Down Expand Up @@ -129,19 +128,6 @@ export const CompactToolGroupDisplay: React.FC<
const overallStatus = getOverallStatus(toolCalls);
const activeTool = getActiveTool(toolCalls);

const isShellCommand = toolCalls.some(
(t) => t.name === SHELL_COMMAND_NAME || t.name === SHELL_NAME,
);
const hasPending = !toolCalls.every(
(t) => t.status === ToolCallStatus.Success,
);

const borderColor = isShellCommand
? theme.ui.symbol
: hasPending
? theme.status.warning
: theme.border.default;

// Take only the first line of description to prevent multi-line shell scripts
// from expanding the compact view (wrap="truncate-end" only handles width overflow,
// not literal \n characters in the content)
Expand All @@ -150,14 +136,7 @@ export const CompactToolGroupDisplay: React.FC<
: '';

return (
<Box
flexDirection="column"
borderStyle="round"
width={contentWidth}
borderDimColor={hasPending}
borderColor={borderColor}
gap={0}
>
<Box flexDirection="column" width={contentWidth} paddingX={1} gap={0}>
{/* Status line: icon + (summary | tool name + description) + count + elapsed */}
<Box flexDirection="row">
<ToolStatusIndicator status={overallStatus} name={activeTool.name} />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -264,13 +264,7 @@ export const InlineParallelAgentsDisplay: React.FC<
const headerLabel = `Parallel agents · ${total} · ${doneCount}/${total} done`;

return (
<Box
flexDirection="column"
borderStyle="round"
width={contentWidth}
borderColor={hasLiveAgent ? theme.status.warning : theme.border.default}
paddingX={1}
>
<Box flexDirection="column" width={contentWidth} paddingX={1}>
<Box>
<Text bold color={theme.text.accent}>
{headerLabel}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ describe('<ToolGroupMessage />', () => {
expect(lastFrame()).toMatchSnapshot();
});

it('renders shell command with yellow border', () => {
it('renders shell command', () => {
const toolCalls = [
createToolCall({
callId: 'shell-1',
Expand Down Expand Up @@ -503,45 +503,6 @@ describe('<ToolGroupMessage />', () => {
});
});

describe('Border Color Logic', () => {
it('uses yellow border when tools are pending', () => {
const toolCalls = [createToolCall({ status: ToolCallStatus.Pending })];
const { lastFrame } = renderWithProviders(
<ToolGroupMessage {...baseProps} toolCalls={toolCalls} />,
);
// The snapshot will capture the visual appearance including border color
expect(lastFrame()).toMatchSnapshot();
});

it('uses yellow border for shell commands even when successful', () => {
const toolCalls = [
createToolCall({
name: 'run_shell_command',
status: ToolCallStatus.Success,
}),
];
const { lastFrame } = renderWithProviders(
<ToolGroupMessage {...baseProps} toolCalls={toolCalls} />,
);
expect(lastFrame()).toMatchSnapshot();
});

it('uses gray border when all tools are successful and no shell commands', () => {
const toolCalls = [
createToolCall({ status: ToolCallStatus.Success }),
createToolCall({
callId: 'tool-2',
name: 'another-tool',
status: ToolCallStatus.Success,
}),
];
const { lastFrame } = renderWithProviders(
<ToolGroupMessage {...baseProps} toolCalls={toolCalls} />,
);
expect(lastFrame()).toMatchSnapshot();
});
});

describe('Height Calculation', () => {
it('calculates available height correctly with multiple tools with results', () => {
const toolCalls = [
Expand Down
54 changes: 9 additions & 45 deletions packages/cli/src/ui/components/messages/ToolGroupMessage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ import { ToolMessage } from './ToolMessage.js';
import { ToolConfirmationMessage } from './ToolConfirmationMessage.js';
import { CompactToolGroupDisplay } from './CompactToolGroupDisplay.js';
import { InlineParallelAgentsDisplay } from './InlineParallelAgentsDisplay.js';
import { theme } from '../../semantic-colors.js';
import { SHELL_COMMAND_NAME, SHELL_NAME } from '../../constants.js';
import { useConfig } from '../../contexts/ConfigContext.js';
import { useCompactMode } from '../../contexts/CompactModeContext.js';
import type { AgentResultDisplay } from '@qwen-code/qwen-code-core';
Expand Down Expand Up @@ -156,7 +154,7 @@ interface ToolGroupMessageProps {
compactLabel?: string;
}

// Main component renders the border and maps the tools using ToolMessage
// Main component maps the tools using ToolMessage
export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({
toolCalls,
availableTerminalHeight,
Expand Down Expand Up @@ -292,15 +290,15 @@ export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({
// Hide the entire group when the live-phase filter leaves nothing
// inline to render — i.e. a pure-running-subagent batch with no
// pending approval. LiveAgentPanel below the composer is the
// single source of truth for those rows; an empty bordered
// container floating above the panel would just be a duplicate
// chrome line. Terminal subagents (completed / failed / cancelled)
// single source of truth for those rows; an empty
// container floating above the panel would just be noise.
// Terminal subagents (completed / failed / cancelled)
// pass through `inlineToolCalls` because `unregisterForeground`'s
// post-delete emit already dropped them from the panel snapshot,
// and the inline path must render `SubagentScrollbackSummary`
// immediately so the user keeps a record of the run.
// (Gate on `isPending` so a degenerate empty `toolCalls=[]` in the
// committed phase still falls through to the legacy empty-border
// committed phase still falls through to the legacy empty-container
// snapshot — the suppression is specifically about live-phase
// panel ownership, not about hiding empty inputs in general.)
if (isPending && inlineToolCalls.length === 0) {
Expand Down Expand Up @@ -345,22 +343,8 @@ export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({
}

// Full expanded view
const hasPending = !inlineToolCalls.every(
(t) => t.status === ToolCallStatus.Success,
);
const isShellCommand = inlineToolCalls.some(
(t) => t.name === SHELL_COMMAND_NAME || t.name === SHELL_NAME,
);
const borderColor =
isShellCommand || isEmbeddedShellFocused
? theme.ui.symbol
: hasPending
? theme.status.warning
: theme.border.default;

const staticHeight = /* border */ 2 + /* marginBottom */ 1;
// account for border (2 chars) and padding (2 chars)
const innerWidth = contentWidth - 4;
const staticHeight = /* marginBottom */ 1;
const innerWidth = contentWidth - 2;

let countToolCallsWithResults = 0;
for (const tool of inlineToolCalls) {
Expand All @@ -385,12 +369,7 @@ export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({
const readCount = memoryReadCount ?? 0;
const writeCount = memoryWriteCount ?? 0;
return (
<Box
flexDirection="column"
borderStyle="round"
width={contentWidth}
borderColor={theme.border.default}
>
<Box flexDirection="column" width={contentWidth}>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] After removing the border, this memory-only branch and the main expanded view (line 393) have no paddingX on their outer <Box>. Meanwhile, CompactToolGroupDisplay correctly gained paddingX={1}. The old borderStyle="round" provided an implicit 1-char left offset that kept content aligned with the compact view. Without it, expanded-view content now sits 1 column to the left of compact-view content.

Suggested change
<Box flexDirection="column" width={contentWidth}>
<Box flexDirection="column" width={contentWidth} paddingX={1}>

Apply the same to line 393: <Box flexDirection="column" width={contentWidth} paddingX={1} gap={0}>.

Note: this would also affect the innerWidth calculation — with outer paddingX={1} (2 chars) plus ToolMessage's own paddingX={1} (2 chars), innerWidth should become contentWidth - 4 again.

— qwen3.7-max via Qwen Code /review

{readCount > 0 && (
<Box paddingLeft={1}>
<Text dimColor>
Expand All @@ -412,22 +391,7 @@ export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({
}

return (

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The old code included a comment explaining that width={contentWidth} protects against an Ink rendering bug where rapidly re-rendering boxes can tear or span extra lines. The border was removed (correctly), but the width constraint is still needed — and the comment explaining why was deleted entirely. A future maintainer who sees width={contentWidth} on a borderless <Box> may remove it as seemingly redundant, re-introducing the rendering glitch.

Consider adding a brief comment:

{/* width={contentWidth} prevents Ink flex overflow during rapid re-renders */}
<Box flexDirection="column" width={contentWidth} gap={0}>

— qwen3.7-max via Qwen Code /review

<Box
flexDirection="column"
borderStyle="round"
/*
This width constraint is highly important and protects us from an Ink rendering bug.
Since the ToolGroup can typically change rendering states frequently, it can cause
Ink to render the border of the box incorrectly and span multiple lines and even
cause tearing.
*/
width={contentWidth}
borderDimColor={
hasPending && (!isShellCommand || !isEmbeddedShellFocused)
}
borderColor={borderColor}
gap={0}
>
<Box flexDirection="column" width={contentWidth} gap={0}>
{/* Memory badge for mixed groups (some memory ops + other ops) */}
{!isMemoryOnlyGroup &&
((memoryWriteCount ?? 0) > 0 || (memoryReadCount ?? 0) > 0) &&
Expand Down
36 changes: 36 additions & 0 deletions packages/cli/src/ui/components/messages/ToolMessage.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,42 @@ describe('<ToolMessage />', () => {
expect(output).not.toContain('MockMarkdown:Test result'); // result hidden
});

it('shows result for Error status in compact mode', () => {
const { lastFrame } = renderWithContext(
<ToolMessage {...baseProps} status={ToolCallStatus.Error} />,
StreamingState.Idle,
true,
);
expect(lastFrame()).toContain('MockMarkdown:Test result');
});

it('shows result for Executing status in compact mode', () => {
const { lastFrame } = renderWithContext(
<ToolMessage {...baseProps} status={ToolCallStatus.Executing} />,
StreamingState.Idle,
true,
);
expect(lastFrame()).toContain('MockMarkdown:Test result');
});

it('shows result for Pending status in compact mode', () => {
const { lastFrame } = renderWithContext(
<ToolMessage {...baseProps} status={ToolCallStatus.Pending} />,
StreamingState.Idle,
true,
);
expect(lastFrame()).toContain('MockMarkdown:Test result');
});

it('shows result when forceShowResult overrides compact collapse', () => {
const { lastFrame } = renderWithContext(
<ToolMessage {...baseProps} forceShowResult />,
StreamingState.Idle,
true,
);
expect(lastFrame()).toContain('MockMarkdown:Test result');
});

describe('ToolStatusIndicator rendering', () => {
it('shows ✓ for Success status', () => {
const { lastFrame } = renderWithContext(
Expand Down
15 changes: 10 additions & 5 deletions packages/cli/src/ui/components/messages/ToolMessage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -670,10 +670,12 @@ export const ToolMessage: React.FC<ToolMessageProps> = ({
// Use the custom hook to determine the display type
const displayRenderer = useResultDisplayRenderer(resultDisplay);
const { compactMode } = useCompactMode();
const effectiveDisplayRenderer =
!compactMode || forceShowResult
? displayRenderer
: { type: 'none' as const };

const isCompleted = status === ToolCallStatus.Success;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] isCompleted is a misleading name — it implies all terminal states (Success, Error, Canceled), but only checks Success. A future maintainer reading shouldCollapse = compactMode && isCompleted && !forceShowResult may assume Error/Canceled are also "completed" and remove the now-redundant forceShowResult for Error status in ToolGroupMessage.tsx, inadvertently causing error results to collapse.

Suggested change
const isCompleted = status === ToolCallStatus.Success;
const isSuccess = status === ToolCallStatus.Success;
const shouldCollapse = compactMode && isSuccess && !forceShowResult;

— qwen3.7-max via Qwen Code /review

const shouldCollapse = compactMode && isCompleted && !forceShowResult;
const effectiveDisplayRenderer = shouldCollapse
Comment thread
chiga0 marked this conversation as resolved.
? { type: 'none' as const }
: displayRenderer;

return (
<Box paddingX={1} paddingY={0} flexDirection="column">
Expand Down Expand Up @@ -780,6 +782,7 @@ const ToolInfo: React.FC<ToolInfo> = ({
status,
emphasis,
}) => {
const { compactMode } = useCompactMode();
const nameColor = React.useMemo<string>(() => {
switch (emphasis) {
case 'high':
Expand All @@ -794,13 +797,15 @@ const ToolInfo: React.FC<ToolInfo> = ({
}
}
}, [emphasis]);
const isDim = compactMode && status === ToolCallStatus.Success;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] isDim only dims Success tools in compact mode, but Canceled is also a terminal, unactionable state. A canceled tool in compact mode currently renders at full brightness + bold + strikethrough, which is more visually noisy than necessary. Consider dimming canceled tools alongside completed ones:

Suggested change
const isDim = compactMode && status === ToolCallStatus.Success;
const isDim = compactMode && (status === ToolCallStatus.Success || status === ToolCallStatus.Canceled);

— qwen3.7-max via Qwen Code /review

return (
Comment thread
chiga0 marked this conversation as resolved.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] isDim is computed independently from shouldCollapse (line 675) and doesn't account for forceShowResult. When a completed tool is force-expanded in compact mode (e.g., terminal subagents via ToolGroupMessage line 462's isTerminalSubagentTool check), the result body is visible but the tool name stays dimmed and unbolded — a visual inconsistency between the header and body.

Suggested change
return (
const isDim = compactMode && status === ToolCallStatus.Success && !forceShowResult;

This requires threading forceShowResult into ToolInfo's props (add it to the interface at line 772). Alternatively, derive isDim from the parent's shouldCollapse and pass it down.

— qwen3.7-max via Qwen Code /review

<Box flexGrow={1}>
<Text
wrap="truncate-end"
strikethrough={status === ToolCallStatus.Canceled}
dimColor={isDim}
>
<Text color={nameColor} bold>
<Text color={nameColor} bold={!isDim}>
{localizeToolDisplayName(name)}
</Text>{' '}
<Text color={theme.text.secondary}>{description}</Text>
Expand Down
Loading
Loading