mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-04-28 11:41:04 +00:00
fix(cli): remove residual blank lines after MCP init completes (#3509)
* fix(cli): remove residual blank lines after MCP init completes (#3095) ConfigInitDisplay rendered <Box marginTop={1}> plus a content line, so the live area grew by 2 rows during startup. When initialization finished and the component unmounted, Ink shrank the live area but the rows it had already committed to the terminal scrollback cannot be reclaimed, leaving a visible gap above the input. Move the MCP init status into the Footer's left-bottom status slot (always mounted, fixed height) so the live area height stays constant across the init → ready transition. The status participates in the existing priority chain: ctrlC / ctrlD / escape / vim / shell / autoAccept / configInit / hint. * fix(cli): suppress MCP init message when custom status line is active Audit follow-up. Previously the configInit branch preceded the suppressHint branch in the footer's left-bottom priority chain. With a custom status line configured, <Text>{null}</Text> collapses to zero rows in Ink, so the footer's bottom row went from 1 row during init to 0 rows after — a 1-row height oscillation that reintroduces the same scrollback-residue symptom the original fix eliminated in the default case. Swap the order so suppressHint short-circuits to null first: the init message now shares the hint's suppression rule, keeping the footer's height constant in every configuration. Also: - Gate the hook's return on isConfigInitialized directly instead of letting the effect clear state, avoiding a one-frame flash where the stale "Initializing..." message leaks through on the first render after init completes. - Cover the new behavior with three Footer tests, including a regression test for the custom-status-line case. * fix(cli): show MCP init progress even under a custom status line Reverting a UX trade-off introduced in the previous commit. That change suppressed the init message whenever a custom status line was active, arguing that <Text>{null}</Text> collapses to zero rows in Ink and any non-zero init row would re-create a one-row shrink on completion. Zero shrink was the wrong goal. Hiding init progress from users who have configured a status line is a real usability loss — the status line does not surface MCP connection state, so those users now see no feedback during startup. A one-time, one-line shrink on init completion is a far smaller regression than the original two-row scrollback residue this PR was created to fix, and strictly better than the silent alternative. Keep the init message in the left-bottom slot and let it sit above suppressHint in the priority chain. Update the regression test so that it pins the new behavior (init is visible with or without a status line) and prevents the suppression from being reintroduced. * fix(cli): keep MCP init progress visible in screen-reader mode Footer is gated behind !isScreenReaderEnabled, so moving the init message inside Footer silenced it for screen-reader users. Render the same message as a plain Text node in Composer when the screen reader is active — screen-reader users don't suffer from the live-area residual row issue that motivated the original move, so an independent node is safe for them. * refactor(cli): drop duplicated screen-reader init path and show progress under YOLO - ScreenReaderAppLayout already mounts <Footer /> directly, so the separate <Text> branch in Composer was producing a duplicated 'Connecting to MCP servers...' line in screen-reader mode. Remove it. - Move configInitMessage ahead of AutoAcceptIndicator in the footer's priority chain so users launched with YOLO / auto-accept-edits still see the ~1s startup progress; the approval-mode indicator takes over as soon as init finishes. - Add unit tests for useConfigInitMessage covering the idle, progress, reset, and unsubscribe paths.
This commit is contained in:
parent
4e0a37549d
commit
3182500835
5 changed files with 183 additions and 21 deletions
|
|
@ -16,7 +16,6 @@ import { useUIActions } from '../contexts/UIActionsContext.js';
|
|||
import { useVimMode } from '../contexts/VimModeContext.js';
|
||||
import { useConfig } from '../contexts/ConfigContext.js';
|
||||
import { StreamingState, type HistoryItemToolGroup } from '../types.js';
|
||||
import { ConfigInitDisplay } from '../components/ConfigInitDisplay.js';
|
||||
import { FeedbackDialog } from '../FeedbackDialog.js';
|
||||
import { t } from '../../i18n/index.js';
|
||||
|
||||
|
|
@ -104,8 +103,6 @@ export const Composer = () => {
|
|||
/>
|
||||
)}
|
||||
|
||||
{!uiState.isConfigInitialized && <ConfigInitDisplay />}
|
||||
|
||||
<QueuedMessageDisplay messageQueue={uiState.messageQueue} />
|
||||
|
||||
{uiState.isFeedbackDialogOpen && <FeedbackDialog />}
|
||||
|
|
|
|||
|
|
@ -78,6 +78,7 @@ const createMockUIState = (overrides: Partial<UIState> = {}): UIState =>
|
|||
contextFileNames: [],
|
||||
showToolDescriptions: false,
|
||||
ideContextState: undefined,
|
||||
isConfigInitialized: true,
|
||||
...overrides,
|
||||
}) as UIState;
|
||||
|
||||
|
|
@ -149,6 +150,43 @@ describe('<Footer />', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('config init message', () => {
|
||||
it('shows init status in place of the hint while config is initializing', () => {
|
||||
const { lastFrame } = renderWithWidth(
|
||||
120,
|
||||
createMockUIState({ isConfigInitialized: false }),
|
||||
);
|
||||
const frame = lastFrame()!;
|
||||
expect(frame).toContain('Initializing...');
|
||||
expect(frame).not.toContain('? for shortcuts');
|
||||
});
|
||||
|
||||
it('falls back to the hint once config is initialized', () => {
|
||||
const { lastFrame } = renderWithWidth(
|
||||
120,
|
||||
createMockUIState({ isConfigInitialized: true }),
|
||||
);
|
||||
const frame = lastFrame()!;
|
||||
expect(frame).not.toContain('Initializing...');
|
||||
expect(frame).toContain('? for shortcuts');
|
||||
});
|
||||
|
||||
// Init progress is more useful than zero layout shift: we show it even
|
||||
// when a custom status line is active, accepting that the row shrinks
|
||||
// by one line once init completes. Still strictly better than the
|
||||
// original bug (a 2-row residual above the input in the default case).
|
||||
it('shows init status even when a custom status line is active', () => {
|
||||
useStatusLineMock.mockReturnValue({ lines: ['model-name ctx:34%'] });
|
||||
const { lastFrame } = renderWithWidth(
|
||||
120,
|
||||
createMockUIState({ isConfigInitialized: false }),
|
||||
);
|
||||
const frame = lastFrame()!;
|
||||
expect(frame).toContain('model-name ctx:34%');
|
||||
expect(frame).toContain('Initializing...');
|
||||
});
|
||||
});
|
||||
|
||||
describe('footer rendering (golden snapshots)', () => {
|
||||
it('renders complete footer on wide terminal', () => {
|
||||
const { lastFrame } = renderWithWidth(120, createMockUIState());
|
||||
|
|
|
|||
|
|
@ -15,10 +15,12 @@ import { ShellModeIndicator } from './ShellModeIndicator.js';
|
|||
import { isNarrowWidth } from '../utils/isNarrowWidth.js';
|
||||
|
||||
import { useStatusLine } from '../hooks/useStatusLine.js';
|
||||
import { useConfigInitMessage } from '../hooks/useConfigInitMessage.js';
|
||||
import { useUIState } from '../contexts/UIStateContext.js';
|
||||
import { useConfig } from '../contexts/ConfigContext.js';
|
||||
import { useVimMode } from '../contexts/VimModeContext.js';
|
||||
import { ApprovalMode } from '@qwen-code/qwen-code-core';
|
||||
import { GeminiSpinner } from './GeminiRespondingSpinner.js';
|
||||
import { t } from '../../i18n/index.js';
|
||||
|
||||
/**
|
||||
|
|
@ -52,6 +54,7 @@ export const Footer: React.FC = () => {
|
|||
const config = useConfig();
|
||||
const { vimEnabled, vimMode } = useVimMode();
|
||||
const { lines: statusLineLines } = useStatusLine();
|
||||
const configInitMessage = useConfigInitMessage(uiState.isConfigInitialized);
|
||||
const dreamRunning = useDreamRunning(config.getProjectRoot());
|
||||
|
||||
const { promptTokenCount, showAutoAcceptIndicator } = {
|
||||
|
|
@ -82,7 +85,15 @@ export const Footer: React.FC = () => {
|
|||
// occupies the footer, so the hint is redundant). Matches upstream behavior.
|
||||
const suppressHint = statusLineLines.length > 0;
|
||||
|
||||
// Left bottom row: high-priority messages > approval mode > hint.
|
||||
// MCP init progress lives in this row (not a standalone component above the
|
||||
// input) so the live area's height is constant in the default case, avoiding
|
||||
// the residual-blank-line artifact left behind when a separate block unmounts.
|
||||
// When a custom status line is active, the row shrinks by 1 on transition to
|
||||
// ready — a one-time, small regression preferred over hiding init progress.
|
||||
//
|
||||
// `configInitMessage` is placed ahead of `showAutoAcceptIndicator` so users
|
||||
// launched with YOLO / auto-accept-edits still see the ~1s startup progress;
|
||||
// the approval-mode indicator takes over as soon as init finishes.
|
||||
const leftBottomContent = uiState.ctrlCPressedOnce ? (
|
||||
<Text color={theme.status.warning}>{t('Press Ctrl+C again to exit.')}</Text>
|
||||
) : uiState.ctrlDPressedOnce ? (
|
||||
|
|
@ -93,6 +104,10 @@ export const Footer: React.FC = () => {
|
|||
<Text color={theme.text.secondary}>-- INSERT --</Text>
|
||||
) : uiState.shellModeActive ? (
|
||||
<ShellModeIndicator />
|
||||
) : configInitMessage ? (
|
||||
<Text color={theme.text.secondary}>
|
||||
<GeminiSpinner /> {configInitMessage}
|
||||
</Text>
|
||||
) : showAutoAcceptIndicator !== undefined &&
|
||||
showAutoAcceptIndicator !== ApprovalMode.DEFAULT ? (
|
||||
<AutoAcceptIndicator approvalMode={showAutoAcceptIndicator} />
|
||||
|
|
|
|||
111
packages/cli/src/ui/hooks/useConfigInitMessage.test.ts
Normal file
111
packages/cli/src/ui/hooks/useConfigInitMessage.test.ts
Normal file
|
|
@ -0,0 +1,111 @@
|
|||
/**
|
||||
* @license
|
||||
* Copyright 2025 Google LLC
|
||||
* SPDX-License-Identifier: Apache-2.0
|
||||
*/
|
||||
|
||||
import { describe, it, expect, afterEach } from 'vitest';
|
||||
import { renderHook, act } from '@testing-library/react';
|
||||
import { MCPServerStatus, type McpClient } from '@qwen-code/qwen-code-core';
|
||||
import { appEvents } from '../../utils/events.js';
|
||||
import { useConfigInitMessage } from './useConfigInitMessage.js';
|
||||
|
||||
function makeClient(status: MCPServerStatus): McpClient {
|
||||
return { getStatus: () => status } as unknown as McpClient;
|
||||
}
|
||||
|
||||
describe('useConfigInitMessage', () => {
|
||||
afterEach(() => {
|
||||
appEvents.removeAllListeners('mcp-client-update');
|
||||
});
|
||||
|
||||
it('returns null once config is initialized', () => {
|
||||
const { result } = renderHook(() => useConfigInitMessage(true));
|
||||
expect(result.current).toBeNull();
|
||||
});
|
||||
|
||||
it('defaults to "Initializing..." while config is still initializing', () => {
|
||||
const { result } = renderHook(() => useConfigInitMessage(false));
|
||||
expect(result.current).toBe('Initializing...');
|
||||
});
|
||||
|
||||
it('reports connection progress as MCP clients connect', () => {
|
||||
const { result } = renderHook(() => useConfigInitMessage(false));
|
||||
|
||||
const clients = new Map<string, McpClient>([
|
||||
['a', makeClient(MCPServerStatus.CONNECTED)],
|
||||
['b', makeClient(MCPServerStatus.DISCONNECTED)],
|
||||
['c', makeClient(MCPServerStatus.DISCONNECTED)],
|
||||
]);
|
||||
|
||||
act(() => {
|
||||
appEvents.emit('mcp-client-update', clients);
|
||||
});
|
||||
expect(result.current).toBe('Connecting to MCP servers... (1/3)');
|
||||
|
||||
clients.set('b', makeClient(MCPServerStatus.CONNECTED));
|
||||
act(() => {
|
||||
appEvents.emit('mcp-client-update', clients);
|
||||
});
|
||||
expect(result.current).toBe('Connecting to MCP servers... (2/3)');
|
||||
});
|
||||
|
||||
it('falls back to "Initializing..." when the clients map is empty', () => {
|
||||
const { result } = renderHook(() => useConfigInitMessage(false));
|
||||
|
||||
act(() => {
|
||||
appEvents.emit(
|
||||
'mcp-client-update',
|
||||
new Map<string, McpClient>([
|
||||
['a', makeClient(MCPServerStatus.CONNECTED)],
|
||||
]),
|
||||
);
|
||||
});
|
||||
expect(result.current).toBe('Connecting to MCP servers... (1/1)');
|
||||
|
||||
act(() => {
|
||||
appEvents.emit('mcp-client-update', new Map<string, McpClient>());
|
||||
});
|
||||
expect(result.current).toBe('Initializing...');
|
||||
});
|
||||
|
||||
it('flips to null as soon as config finishes initializing', () => {
|
||||
const { result, rerender } = renderHook(
|
||||
({ initialized }: { initialized: boolean }) =>
|
||||
useConfigInitMessage(initialized),
|
||||
{ initialProps: { initialized: false } },
|
||||
);
|
||||
|
||||
act(() => {
|
||||
appEvents.emit(
|
||||
'mcp-client-update',
|
||||
new Map<string, McpClient>([
|
||||
['a', makeClient(MCPServerStatus.CONNECTED)],
|
||||
]),
|
||||
);
|
||||
});
|
||||
expect(result.current).toBe('Connecting to MCP servers... (1/1)');
|
||||
|
||||
rerender({ initialized: true });
|
||||
expect(result.current).toBeNull();
|
||||
});
|
||||
|
||||
it('unsubscribes from mcp-client-update on unmount', () => {
|
||||
const { unmount } = renderHook(() => useConfigInitMessage(false));
|
||||
expect(appEvents.listenerCount('mcp-client-update')).toBe(1);
|
||||
unmount();
|
||||
expect(appEvents.listenerCount('mcp-client-update')).toBe(0);
|
||||
});
|
||||
|
||||
it('unsubscribes when config transitions to initialized', () => {
|
||||
const { rerender } = renderHook(
|
||||
({ initialized }: { initialized: boolean }) =>
|
||||
useConfigInitMessage(initialized),
|
||||
{ initialProps: { initialized: false } },
|
||||
);
|
||||
expect(appEvents.listenerCount('mcp-client-update')).toBe(1);
|
||||
|
||||
rerender({ initialized: true });
|
||||
expect(appEvents.listenerCount('mcp-client-update')).toBe(0);
|
||||
});
|
||||
});
|
||||
|
|
@ -5,19 +5,23 @@
|
|||
*/
|
||||
|
||||
import { useEffect, useState } from 'react';
|
||||
import { appEvents } from './../../utils/events.js';
|
||||
import { Box, Text } from 'ink';
|
||||
import { useConfig } from '../contexts/ConfigContext.js';
|
||||
import { appEvents } from '../../utils/events.js';
|
||||
import { type McpClient, MCPServerStatus } from '@qwen-code/qwen-code-core';
|
||||
import { GeminiSpinner } from './GeminiRespondingSpinner.js';
|
||||
import { theme } from '../semantic-colors.js';
|
||||
import { t } from '../../i18n/index.js';
|
||||
|
||||
export const ConfigInitDisplay = () => {
|
||||
const config = useConfig();
|
||||
const [message, setMessage] = useState(t('Initializing...'));
|
||||
// Tracks MCP connection progress. Returns the current status string while
|
||||
// config is initializing, or `null` once complete so callers can fall
|
||||
// through to their default content.
|
||||
export function useConfigInitMessage(
|
||||
isConfigInitialized: boolean,
|
||||
): string | null {
|
||||
const [message, setMessage] = useState<string>(() => t('Initializing...'));
|
||||
|
||||
useEffect(() => {
|
||||
if (isConfigInitialized) {
|
||||
return;
|
||||
}
|
||||
|
||||
const onChange = (clients?: Map<string, McpClient>) => {
|
||||
if (!clients || clients.size === 0) {
|
||||
setMessage(t('Initializing...'));
|
||||
|
|
@ -41,13 +45,10 @@ export const ConfigInitDisplay = () => {
|
|||
return () => {
|
||||
appEvents.off('mcp-client-update', onChange);
|
||||
};
|
||||
}, [config]);
|
||||
}, [isConfigInitialized]);
|
||||
|
||||
return (
|
||||
<Box marginTop={1}>
|
||||
<Text>
|
||||
<GeminiSpinner /> <Text color={theme.text.primary}>{message}</Text>
|
||||
</Text>
|
||||
</Box>
|
||||
);
|
||||
};
|
||||
// Gating on isConfigInitialized (rather than clearing state from the effect)
|
||||
// ensures the first render that flips to initialized returns null without
|
||||
// a transient frame still showing the old message.
|
||||
return isConfigInitialized ? null : message;
|
||||
}
|
||||
Loading…
Add table
Add a link
Reference in a new issue