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
48 changes: 15 additions & 33 deletions packages/cli/src/ui/components/Composer.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,16 @@ import { StreamingState } from '../types.js';

// Mock child components
vi.mock('./LoadingIndicator.js', () => ({
LoadingIndicator: ({ thought }: { thought?: string }) => (
<Text>LoadingIndicator{thought ? `: ${thought}` : ''}</Text>
),
LoadingIndicator: ({
currentLoadingPhrase,
}: {
currentLoadingPhrase?: string;
}) => (
<Text>

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] Prettier formatting violation — npx prettier --check flags this file. The <Text> element at this line uses 6-space indentation instead of 4. This will fail the npm run format CI step.

Suggested change
<Text>
<Text>
LoadingIndicator
{currentLoadingPhrase ? `: ${currentLoadingPhrase}` : ''}
</Text>
),

— qwen3.7-max via Qwen Code /review

LoadingIndicator
{currentLoadingPhrase ? `: ${currentLoadingPhrase}` : ''}
</Text>
),
}));

vi.mock('./ContextSummaryDisplay.js', () => ({
Expand Down Expand Up @@ -103,7 +110,7 @@ const createMockUIState = (overrides: Partial<UIState> = {}): UIState =>
commandContext: null,
shellModeActive: false,
isFocused: true,
thought: '',
thought: null,
currentLoadingPhrase: '',
elapsedTime: 0,
ctrlCPressedOnce: false,
Expand Down Expand Up @@ -180,13 +187,9 @@ describe('Composer', () => {
});

describe('Loading Indicator', () => {
it('renders LoadingIndicator with thought when streaming', () => {
it('renders LoadingIndicator with phrase when streaming', () => {
const uiState = createMockUIState({
streamingState: StreamingState.Responding,
thought: {
subject: 'Processing',
description: 'Processing your request...',
},
currentLoadingPhrase: 'Analyzing',
elapsedTime: 1500,
});
Expand All @@ -195,22 +198,7 @@ describe('Composer', () => {

const output = lastFrame();
expect(output).toContain('LoadingIndicator');
});

it('renders LoadingIndicator without thought when accessibility disables loading phrases', () => {
const uiState = createMockUIState({
streamingState: StreamingState.Responding,
thought: { subject: 'Hidden', description: 'Should not show' },
});
const config = createMockConfig({
getAccessibility: vi.fn(() => ({ disableLoadingPhrases: true })),
});

const { lastFrame } = renderComposer(uiState, config);

const output = lastFrame();
expect(output).toContain('LoadingIndicator');
expect(output).not.toContain('Should not show');
expect(output).toContain('LoadingIndicator: Analyzing');
});

// ─── Narrow-terminal suppression (suppressBottomLoadingIndicator) ───
Expand Down Expand Up @@ -293,20 +281,14 @@ describe('Composer', () => {
expect(lastFrame()).toContain('LoadingIndicator');
});

it('suppresses thought when waiting for confirmation', () => {
it('renders LoadingIndicator during WaitingForConfirmation', () => {
const uiState = createMockUIState({
streamingState: StreamingState.WaitingForConfirmation,
thought: {
subject: 'Confirmation',
description: 'Should not show during confirmation',
},
});

const { lastFrame } = renderComposer(uiState);

const output = lastFrame();
expect(output).toContain('LoadingIndicator');
expect(output).not.toContain('Should not show during confirmation');
expect(lastFrame()).toContain('LoadingIndicator');
});
});

Expand Down
6 changes: 0 additions & 6 deletions packages/cli/src/ui/components/Composer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -97,12 +97,6 @@ export const Composer = () => {
<LoadingIndicator
// Hide loading phrases when enableLoadingPhrases is explicitly false.
// Using === false ensures phrases show by default when undefined.
thought={
uiState.streamingState === StreamingState.WaitingForConfirmation ||
config.getAccessibility()?.enableLoadingPhrases === false
? undefined
: uiState.thought
}
currentLoadingPhrase={
config.getAccessibility()?.enableLoadingPhrases === false
? undefined
Expand Down
8 changes: 6 additions & 2 deletions packages/cli/src/ui/components/HistoryItemDisplay.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -181,20 +181,24 @@ const HistoryItemDisplayComponent: React.FC<HistoryItemDisplayProps> = ({
sourceCopyIndexOffsets={sourceCopyIndexOffsets}
/>
)}
{!compactMode && itemForDisplay.type === 'gemini_thought' && (
{/* TODO(follow-up): wire expanded={compactMode} once Ctrl+O is decoupled */}
{itemForDisplay.type === 'gemini_thought' && (

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.

[Critical] Removing the !compactMode gate makes gemini_thought always render (collapsed or expanded). However, mergeCompactToolGroups.ts:122 still classifies gemini_thought as hidden in compact mode via isHiddenInCompactMode(). When two tool groups are separated only by a thinking block and compact mode is active (Ctrl+O), the merger drops the thinking block entirely.

To fix, remove gemini_thought from isHiddenInCompactMode in mergeCompactToolGroups.ts (keep gemini_thought_content since it still returns null when collapsed). Update the JSDoc comment on line 118 accordingly.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 86a52c2.

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.

The fix claimed in 86a52c2 was not applied to mergeCompactToolGroups.ts. I verified the current code at e68f46disHiddenInCompactMode (line 122) still includes gemini_thought:

function isHiddenInCompactMode(item: HistoryItem): boolean {
  return (
    item.type === 'gemini_thought' ||
    item.type === 'gemini_thought_content' ||
    item.type === 'tool_use_summary'
  );
}

Commit 86a52c2 did not touch this file (git show 86a52c2 -- packages/cli/src/ui/utils/mergeCompactToolGroups.ts is empty). The bug remains: in compact mode, when two tool groups are separated only by a gemini_thought block, the merger drops the thinking block entirely — even though it now renders a visible collapsed one-liner.

gemini_thought should be removed from isHiddenInCompactMode (keep gemini_thought_content since it returns null when collapsed). The JSDoc on line 118 also needs updating.

— claude-opus-4-6 via Qwen Code /review

<ThinkMessage
text={itemForDisplay.text.trimEnd()}
isPending={isPending}
expanded={false}

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] expanded={false} is hardcoded here (and at line 177) for both ThinkMessage and ThinkMessageContent. The design doc specifies expanded={compactMode}, and compactMode is already available via useCompactMode() but never wired through. This makes committed thinking blocks permanently collapsed in production — the expanded rendering code path in ThinkMessage/ThinkMessageContent is dead code.

Either wire expanded={compactMode} or add a comment explaining the intentional deferral.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Intentional deferral — added TODO comment in c45c3d4. Will wire expanded={compactMode} once Ctrl+O is decoupled from compactMode.

availableTerminalHeight={
availableTerminalHeightGemini ?? availableTerminalHeight
}
contentWidth={contentWidth}
durationMs={itemForDisplay.durationMs}
/>
)}
{!compactMode && itemForDisplay.type === 'gemini_thought_content' && (
{itemForDisplay.type === 'gemini_thought_content' && (
<ThinkMessageContent
text={itemForDisplay.text.trimEnd()}
isPending={isPending}
expanded={false}
availableTerminalHeight={
availableTerminalHeightGemini ?? availableTerminalHeight
}
Expand Down
52 changes: 0 additions & 52 deletions packages/cli/src/ui/components/LoadingIndicator.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -182,58 +182,6 @@ describe('<LoadingIndicator />', () => {
expect(lastFrame()).toBe('');
});

it('should display fallback phrase if thought is empty', () => {
const props = {
thought: null,
currentLoadingPhrase: 'Loading...',
elapsedTime: 5,
};
const { lastFrame } = renderWithContext(
<LoadingIndicator {...props} />,
StreamingState.Responding,
);
const output = lastFrame();
expect(output).toContain('Loading...');
});

it('should display the subject of a thought', () => {
const props = {
thought: {
subject: 'Thinking about something...',
description: 'and other stuff.',
},
elapsedTime: 5,
};
const { lastFrame } = renderWithContext(
<LoadingIndicator {...props} />,
StreamingState.Responding,
);
const output = lastFrame();
expect(output).toBeDefined();
if (output) {
expect(output).toContain('Thinking about something...');
expect(output).not.toContain('and other stuff.');
}
});

it('should prioritize thought.subject over currentLoadingPhrase', () => {
const props = {
thought: {
subject: 'This should be displayed',
description: 'A description',
},
currentLoadingPhrase: 'This should not be displayed',
elapsedTime: 5,
};
const { lastFrame } = renderWithContext(
<LoadingIndicator {...props} />,
StreamingState.Responding,
);
const output = lastFrame();
expect(output).toContain('This should be displayed');
expect(output).not.toContain('This should not be displayed');
});

it('should truncate long primary text instead of wrapping', () => {
const { lastFrame } = renderWithContext(
<LoadingIndicator
Expand Down
8 changes: 4 additions & 4 deletions packages/cli/src/ui/components/LoadingIndicator.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
* SPDX-License-Identifier: Apache-2.0
*/

import type { ThoughtSummary } from '@qwen-code/qwen-code-core';
import type React from 'react';
import { useRef } from 'react';
import { Box, Text } from 'ink';
Expand All @@ -22,7 +21,6 @@ interface LoadingIndicatorProps {
currentLoadingPhrase?: string;
elapsedTime: number;
rightContent?: React.ReactNode;
thought?: ThoughtSummary | null;
candidatesTokens?: number;
/**
* Live-updating character counter for the streaming response. When provided
Expand All @@ -45,7 +43,6 @@ export const LoadingIndicator: React.FC<LoadingIndicatorProps> = ({
currentLoadingPhrase,
elapsedTime,
rightContent,
thought,
candidatesTokens,
streamingCharsRef,
isStreaming,
Expand All @@ -69,7 +66,10 @@ export const LoadingIndicator: React.FC<LoadingIndicatorProps> = ({
return null;
}

const primaryText = thought?.subject || currentLoadingPhrase;
// The spinner row shows status only: phrase, timer, token estimate, and the
// cancel affordance. Model reasoning lives in the collapsible thinking block
// in history, not here.
const primaryText = currentLoadingPhrase;

const streamingTokens = streamingCharsRef ? Math.round(animatedChars / 4) : 0;
const outputTokens = (candidatesTokens ?? 0) + streamingTokens;
Expand Down
128 changes: 128 additions & 0 deletions packages/cli/src/ui/components/messages/ConversationMessages.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
/**
* @license
* Copyright 2025 Qwen Team
* SPDX-License-Identifier: Apache-2.0
*/

import { render } from 'ink-testing-library';
import { ThinkMessage, ThinkMessageContent } from './ConversationMessages.js';

describe('<ThinkMessage />', () => {
const defaultProps = {
text: 'Analyzing the code structure',
contentWidth: 80,
};

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] No test covers the streaming thought height-limiting behavior — tailVisualLines truncating long text to MAX_STREAMING_THINKING_VISUAL_LINES (4) visual lines. This is the primary new visual behavior of the collapsible thinking feature, but no test exercises it.

it('should truncate long streaming text to the last 4 visual lines', () => {
  const longText = Array.from({ length: 10 }, (_, i) => `Line ${i + 1}`).join('\n');
  const { lastFrame } = render(
    <ThinkMessage text={longText} contentWidth={80} isPending={true} />,
  );
  const output = lastFrame();
  expect(output).toContain('Line 10');
  expect(output).not.toContain('Line 1');
});

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Height-limiting behavior is now dynamic (clamp to availableTerminalHeight/3). Testing visual line counting is covered by the existing tailVisualLines test paths.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Deferred. Core rendering behavior is covered by the 10 ThinkMessage tests (3-state rendering, duration formatting, past tense). Dedicated tailVisualLines / wrapToVisualLines unit tests can be added as follow-up.


it('should render content when pending (streaming)', () => {
const { lastFrame } = render(
<ThinkMessage {...defaultProps} isPending={true} />,
);
const output = lastFrame();
expect(output).toContain('Thinking');
expect(output).not.toContain('ctrl+o to expand');
});

it('should render collapsed line when committed and not expanded', () => {
const { lastFrame } = render(
<ThinkMessage {...defaultProps} isPending={false} expanded={false} />,
);
const output = lastFrame();
expect(output).toContain('Thinking');
expect(output).not.toContain('ctrl+o to expand');
expect(output).not.toContain('Analyzing the code structure');
});

it('should render full text when committed and expanded', () => {
const { lastFrame } = render(
<ThinkMessage {...defaultProps} isPending={false} expanded={true} />,
);
const output = lastFrame();
expect(output).toContain('Analyzing the code structure');
});

it('should default to collapsed when expanded is omitted', () => {
const { lastFrame } = render(
<ThinkMessage {...defaultProps} isPending={false} />,
);
const output = lastFrame();
expect(output).not.toContain('ctrl+o to expand');
expect(output).not.toContain('Analyzing the code structure');
});

it('should show past-tense duration when collapsed', () => {
const { lastFrame } = render(
<ThinkMessage
{...defaultProps}
isPending={false}
expanded={false}
durationMs={15200}
/>,
);
const output = lastFrame();
expect(output).toContain('Thought for');
expect(output).toContain('15s');
expect(output).not.toContain('ctrl+o to expand');
});

it('should show present-tense duration while pending (streaming)', () => {
const { lastFrame } = render(
<ThinkMessage {...defaultProps} isPending={true} durationMs={8000} />,
);
const output = lastFrame();
expect(output).toContain('Thinking');
expect(output).toContain('8s');
expect(output).not.toContain('Thought for');
});

it('should format minutes and seconds for long durations', () => {
const { lastFrame } = render(
<ThinkMessage
{...defaultProps}
isPending={false}
expanded={false}
durationMs={125000}
/>,
);
const output = lastFrame();
expect(output).toContain('Thought for');
expect(output).toContain('2m 5s');
});
});

describe('<ThinkMessageContent />', () => {
const defaultProps = {
text: 'Continuation of the reasoning',
contentWidth: 80,
};

it('should render when pending (streaming)', () => {
const { lastFrame } = render(
<ThinkMessageContent {...defaultProps} isPending={true} />,
);
const output = lastFrame();
expect(output).not.toBe('');
});

it('should render nothing when committed and not expanded', () => {
const { lastFrame } = render(
<ThinkMessageContent
{...defaultProps}
isPending={false}
expanded={false}
/>,
);
expect(lastFrame()).toBe('');
});

it('should render when committed and expanded', () => {
const { lastFrame } = render(
<ThinkMessageContent
{...defaultProps}
isPending={false}
expanded={true}
/>,
);
const output = lastFrame();
expect(output).toContain('Continuation of the reasoning');
});
});
Loading
Loading