From e506b40c271da0e05a361f5299c37976a9e1f1f3 Mon Sep 17 00:00:00 2001 From: Pyush Sinha <89475668+psinha40898@users.noreply.github.com> Date: Mon, 4 Aug 2025 09:53:50 -0700 Subject: [PATCH] fix: /help remove flickering and respect clear shortcut (ctr+l) (#3611) Co-authored-by: Jacob Richman Co-authored-by: Allen Hutchison --- packages/cli/src/ui/App.tsx | 7 +-- .../cli/src/ui/commands/helpCommand.test.ts | 46 ++++++++++++------- packages/cli/src/ui/commands/helpCommand.ts | 16 ++++--- packages/cli/src/ui/commands/types.ts | 2 +- .../src/ui/components/HistoryItemDisplay.tsx | 5 ++ .../ui/hooks/slashCommandProcessor.test.ts | 21 ++++----- .../cli/src/ui/hooks/slashCommandProcessor.ts | 11 ++--- .../cli/src/ui/hooks/useGeminiStream.test.tsx | 19 -------- packages/cli/src/ui/hooks/useGeminiStream.ts | 3 -- packages/cli/src/ui/types.ts | 12 +++++ 10 files changed, 72 insertions(+), 70 deletions(-) diff --git a/packages/cli/src/ui/App.tsx b/packages/cli/src/ui/App.tsx index f63fcb35..3b695111 100644 --- a/packages/cli/src/ui/App.tsx +++ b/packages/cli/src/ui/App.tsx @@ -38,7 +38,6 @@ import { AuthInProgress } from './components/AuthInProgress.js'; import { EditorSettingsDialog } from './components/EditorSettingsDialog.js'; import { ShellConfirmationDialog } from './components/ShellConfirmationDialog.js'; import { Colors } from './colors.js'; -import { Help } from './components/Help.js'; import { loadHierarchicalGeminiMemory } from '../config/config.js'; import { LoadedSettings } from '../config/settings.js'; import { Tips } from './components/Tips.js'; @@ -146,7 +145,6 @@ const App = ({ config, settings, startupWarnings = [], version }: AppProps) => { const [geminiMdFileCount, setGeminiMdFileCount] = useState(0); const [debugMessage, setDebugMessage] = useState(''); - const [showHelp, setShowHelp] = useState(false); const [themeError, setThemeError] = useState(null); const [authError, setAuthError] = useState(null); const [editorError, setEditorError] = useState(null); @@ -473,7 +471,6 @@ const App = ({ config, settings, startupWarnings = [], version }: AppProps) => { clearItems, loadHistory, refreshStatic, - setShowHelp, setDebugMessage, openThemeDialog, openAuthDialog, @@ -495,7 +492,6 @@ const App = ({ config, settings, startupWarnings = [], version }: AppProps) => { config.getGeminiClient(), history, addItem, - setShowHelp, config, setDebugMessage, handleSlashCommand, @@ -802,6 +798,7 @@ const App = ({ config, settings, startupWarnings = [], version }: AppProps) => { item={h} isPending={false} config={config} + commands={slashCommands} /> )), ]} @@ -829,8 +826,6 @@ const App = ({ config, settings, startupWarnings = [], version }: AppProps) => { - {showHelp && } - {/* Move UpdateNotification to render update notification above input area */} {updateInfo && } diff --git a/packages/cli/src/ui/commands/helpCommand.test.ts b/packages/cli/src/ui/commands/helpCommand.test.ts index b0441106..566eead7 100644 --- a/packages/cli/src/ui/commands/helpCommand.test.ts +++ b/packages/cli/src/ui/commands/helpCommand.test.ts @@ -4,37 +4,49 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { vi, describe, it, expect, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { helpCommand } from './helpCommand.js'; import { type CommandContext } from './types.js'; +import { createMockCommandContext } from '../../test-utils/mockCommandContext.js'; +import { MessageType } from '../types.js'; +import { CommandKind } from './types.js'; describe('helpCommand', () => { let mockContext: CommandContext; + const originalEnv = { ...process.env }; beforeEach(() => { - mockContext = {} as unknown as CommandContext; + mockContext = createMockCommandContext({ + ui: { + addItem: vi.fn(), + }, + } as unknown as CommandContext); }); - it("should return a dialog action and log a debug message for '/help'", () => { - const consoleDebugSpy = vi - .spyOn(console, 'debug') - .mockImplementation(() => {}); + afterEach(() => { + process.env = { ...originalEnv }; + vi.clearAllMocks(); + }); + + it('should add a help message to the UI history', async () => { if (!helpCommand.action) { throw new Error('Help command has no action'); } - const result = helpCommand.action(mockContext, ''); - expect(result).toEqual({ - type: 'dialog', - dialog: 'help', - }); - expect(consoleDebugSpy).toHaveBeenCalledWith('Opening help UI ...'); + await helpCommand.action(mockContext, ''); + + expect(mockContext.ui.addItem).toHaveBeenCalledWith( + expect.objectContaining({ + type: MessageType.HELP, + timestamp: expect.any(Date), + }), + expect.any(Number), + ); }); - it("should also be triggered by its alternative name '?'", () => { - // This test is more conceptual. The routing of altNames to the command - // is handled by the slash command processor, but we can assert the - // altNames is correctly defined on the command object itself. - expect(helpCommand.altNames).toContain('?'); + it('should have the correct command properties', () => { + expect(helpCommand.name).toBe('help'); + expect(helpCommand.kind).toBe(CommandKind.BUILT_IN); + expect(helpCommand.description).toBe('for help on gemini-cli'); }); }); diff --git a/packages/cli/src/ui/commands/helpCommand.ts b/packages/cli/src/ui/commands/helpCommand.ts index 03c64615..0b71ce8f 100644 --- a/packages/cli/src/ui/commands/helpCommand.ts +++ b/packages/cli/src/ui/commands/helpCommand.ts @@ -4,18 +4,20 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { CommandKind, OpenDialogActionReturn, SlashCommand } from './types.js'; +import { CommandKind, SlashCommand } from './types.js'; +import { MessageType, type HistoryItemHelp } from '../types.js'; export const helpCommand: SlashCommand = { name: 'help', altNames: ['?'], - description: 'for help on gemini-cli', kind: CommandKind.BUILT_IN, - action: (_context, _args): OpenDialogActionReturn => { - console.debug('Opening help UI ...'); - return { - type: 'dialog', - dialog: 'help', + description: 'for help on gemini-cli', + action: async (context) => { + const helpItem: Omit = { + type: MessageType.HELP, + timestamp: new Date(), }; + + context.ui.addItem(helpItem, Date.now()); }, }; diff --git a/packages/cli/src/ui/commands/types.ts b/packages/cli/src/ui/commands/types.ts index 900be866..2de221f0 100644 --- a/packages/cli/src/ui/commands/types.ts +++ b/packages/cli/src/ui/commands/types.ts @@ -98,7 +98,7 @@ export interface MessageActionReturn { */ export interface OpenDialogActionReturn { type: 'dialog'; - dialog: 'help' | 'auth' | 'theme' | 'editor' | 'privacy'; + dialog: 'auth' | 'theme' | 'editor' | 'privacy'; } /** diff --git a/packages/cli/src/ui/components/HistoryItemDisplay.tsx b/packages/cli/src/ui/components/HistoryItemDisplay.tsx index eba4ea47..74615b26 100644 --- a/packages/cli/src/ui/components/HistoryItemDisplay.tsx +++ b/packages/cli/src/ui/components/HistoryItemDisplay.tsx @@ -21,6 +21,8 @@ import { ModelStatsDisplay } from './ModelStatsDisplay.js'; import { ToolStatsDisplay } from './ToolStatsDisplay.js'; import { SessionSummaryDisplay } from './SessionSummaryDisplay.js'; import { Config } from '@google/gemini-cli-core'; +import { Help } from './Help.js'; +import { SlashCommand } from '../commands/types.js'; interface HistoryItemDisplayProps { item: HistoryItem; @@ -29,6 +31,7 @@ interface HistoryItemDisplayProps { isPending: boolean; config?: Config; isFocused?: boolean; + commands?: readonly SlashCommand[]; } export const HistoryItemDisplay: React.FC = ({ @@ -37,6 +40,7 @@ export const HistoryItemDisplay: React.FC = ({ terminalWidth, isPending, config, + commands, isFocused = true, }) => ( @@ -71,6 +75,7 @@ export const HistoryItemDisplay: React.FC = ({ gcpProject={item.gcpProject} /> )} + {item.type === 'help' && commands && } {item.type === 'stats' && } {item.type === 'model_stats' && } {item.type === 'tool_stats' && } diff --git a/packages/cli/src/ui/hooks/slashCommandProcessor.test.ts b/packages/cli/src/ui/hooks/slashCommandProcessor.test.ts index d9fe8530..a37af262 100644 --- a/packages/cli/src/ui/hooks/slashCommandProcessor.test.ts +++ b/packages/cli/src/ui/hooks/slashCommandProcessor.test.ts @@ -90,7 +90,7 @@ describe('useSlashCommandProcessor', () => { const mockAddItem = vi.fn(); const mockClearItems = vi.fn(); const mockLoadHistory = vi.fn(); - const mockSetShowHelp = vi.fn(); + const mockOpenThemeDialog = vi.fn(); const mockOpenAuthDialog = vi.fn(); const mockSetQuittingMessages = vi.fn(); @@ -132,9 +132,8 @@ describe('useSlashCommandProcessor', () => { mockClearItems, mockLoadHistory, vi.fn(), // refreshStatic - mockSetShowHelp, vi.fn(), // onDebugMessage - vi.fn(), // openThemeDialog + mockOpenThemeDialog, // openThemeDialog mockOpenAuthDialog, vi.fn(), // openEditorDialog vi.fn(), // toggleCorgiMode @@ -334,19 +333,19 @@ describe('useSlashCommandProcessor', () => { }); describe('Action Result Handling', () => { - it('should handle "dialog: help" action', async () => { + it('should handle "dialog: theme" action', async () => { const command = createTestCommand({ - name: 'helpcmd', - action: vi.fn().mockResolvedValue({ type: 'dialog', dialog: 'help' }), + name: 'themecmd', + action: vi.fn().mockResolvedValue({ type: 'dialog', dialog: 'theme' }), }); const result = setupProcessorHook([command]); await waitFor(() => expect(result.current.slashCommands).toHaveLength(1)); await act(async () => { - await result.current.handleSlashCommand('/helpcmd'); + await result.current.handleSlashCommand('/themecmd'); }); - expect(mockSetShowHelp).toHaveBeenCalledWith(true); + expect(mockOpenThemeDialog).toHaveBeenCalled(); }); it('should handle "load_history" action', async () => { @@ -819,15 +818,15 @@ describe('useSlashCommandProcessor', () => { mockClearItems, mockLoadHistory, vi.fn(), // refreshStatic - mockSetShowHelp, vi.fn(), // onDebugMessage vi.fn(), // openThemeDialog mockOpenAuthDialog, - vi.fn(), // openEditorDialog, + vi.fn(), // openEditorDialog vi.fn(), // toggleCorgiMode mockSetQuittingMessages, vi.fn(), // openPrivacyNotice - vi.fn(), // toggleVimEnabled + vi.fn().mockResolvedValue(false), // toggleVimEnabled + vi.fn(), // setIsProcessing ), ); diff --git a/packages/cli/src/ui/hooks/slashCommandProcessor.ts b/packages/cli/src/ui/hooks/slashCommandProcessor.ts index a2a1837d..6d9f4643 100644 --- a/packages/cli/src/ui/hooks/slashCommandProcessor.ts +++ b/packages/cli/src/ui/hooks/slashCommandProcessor.ts @@ -42,7 +42,6 @@ export const useSlashCommandProcessor = ( clearItems: UseHistoryManagerReturn['clearItems'], loadHistory: UseHistoryManagerReturn['loadHistory'], refreshStatic: () => void, - setShowHelp: React.Dispatch>, onDebugMessage: (message: string) => void, openThemeDialog: () => void, openAuthDialog: () => void, @@ -105,6 +104,11 @@ export const useSlashCommandProcessor = ( selectedAuthType: message.selectedAuthType, gcpProject: message.gcpProject, }; + } else if (message.type === MessageType.HELP) { + historyItemContent = { + type: 'help', + timestamp: message.timestamp, + }; } else if (message.type === MessageType.STATS) { historyItemContent = { type: 'stats', @@ -138,7 +142,6 @@ export const useSlashCommandProcessor = ( }, [addItem], ); - const commandContext = useMemo( (): CommandContext => ({ services: { @@ -333,9 +336,6 @@ export const useSlashCommandProcessor = ( return { type: 'handled' }; case 'dialog': switch (result.dialog) { - case 'help': - setShowHelp(true); - return { type: 'handled' }; case 'auth': openAuthDialog(); return { type: 'handled' }; @@ -462,7 +462,6 @@ export const useSlashCommandProcessor = ( [ config, addItem, - setShowHelp, openAuthDialog, commands, commandContext, diff --git a/packages/cli/src/ui/hooks/useGeminiStream.test.tsx b/packages/cli/src/ui/hooks/useGeminiStream.test.tsx index 085e3e96..062c1687 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.test.tsx +++ b/packages/cli/src/ui/hooks/useGeminiStream.test.tsx @@ -30,7 +30,6 @@ import { SlashCommandProcessorResult, StreamingState, } from '../types.js'; -import { Dispatch, SetStateAction } from 'react'; import { LoadedSettings } from '../../config/settings.js'; // --- MOCKS --- @@ -257,7 +256,6 @@ describe('mergePartListUnions', () => { // --- Tests for useGeminiStream Hook --- describe('useGeminiStream', () => { let mockAddItem: Mock; - let mockSetShowHelp: Mock; let mockConfig: Config; let mockOnDebugMessage: Mock; let mockHandleSlashCommand: Mock; @@ -269,7 +267,6 @@ describe('useGeminiStream', () => { vi.clearAllMocks(); // Clear mocks before each test mockAddItem = vi.fn(); - mockSetShowHelp = vi.fn(); // Define the mock for getGeminiClient const mockGetGeminiClient = vi.fn().mockImplementation(() => { // MockedGeminiClientClass is defined in the module scope by the previous change. @@ -382,7 +379,6 @@ describe('useGeminiStream', () => { client: any; history: HistoryItem[]; addItem: UseHistoryManagerReturn['addItem']; - setShowHelp: Dispatch>; config: Config; onDebugMessage: (message: string) => void; handleSlashCommand: ( @@ -400,7 +396,6 @@ describe('useGeminiStream', () => { props.client, props.history, props.addItem, - props.setShowHelp, props.config, props.onDebugMessage, props.handleSlashCommand, @@ -417,7 +412,6 @@ describe('useGeminiStream', () => { client, history: [], addItem: mockAddItem as unknown as UseHistoryManagerReturn['addItem'], - setShowHelp: mockSetShowHelp, config: mockConfig, onDebugMessage: mockOnDebugMessage, handleSlashCommand: mockHandleSlashCommand as unknown as ( @@ -542,7 +536,6 @@ describe('useGeminiStream', () => { new MockedGeminiClientClass(mockConfig), [], mockAddItem, - mockSetShowHelp, mockConfig, mockOnDebugMessage, mockHandleSlashCommand, @@ -610,7 +603,6 @@ describe('useGeminiStream', () => { client, [], mockAddItem, - mockSetShowHelp, mockConfig, mockOnDebugMessage, mockHandleSlashCommand, @@ -707,7 +699,6 @@ describe('useGeminiStream', () => { client, [], mockAddItem, - mockSetShowHelp, mockConfig, mockOnDebugMessage, mockHandleSlashCommand, @@ -810,7 +801,6 @@ describe('useGeminiStream', () => { new MockedGeminiClientClass(mockConfig), [], mockAddItem, - mockSetShowHelp, mockConfig, mockOnDebugMessage, mockHandleSlashCommand, @@ -1161,7 +1151,6 @@ describe('useGeminiStream', () => { new MockedGeminiClientClass(mockConfig), [], mockAddItem, - mockSetShowHelp, mockConfig, mockOnDebugMessage, mockHandleSlashCommand, @@ -1213,7 +1202,6 @@ describe('useGeminiStream', () => { new MockedGeminiClientClass(testConfig), [], mockAddItem, - mockSetShowHelp, testConfig, mockOnDebugMessage, mockHandleSlashCommand, @@ -1262,7 +1250,6 @@ describe('useGeminiStream', () => { new MockedGeminiClientClass(mockConfig), [], mockAddItem, - mockSetShowHelp, mockConfig, mockOnDebugMessage, mockHandleSlashCommand, @@ -1309,7 +1296,6 @@ describe('useGeminiStream', () => { new MockedGeminiClientClass(mockConfig), [], mockAddItem, - mockSetShowHelp, mockConfig, mockOnDebugMessage, mockHandleSlashCommand, @@ -1357,7 +1343,6 @@ describe('useGeminiStream', () => { new MockedGeminiClientClass(mockConfig), [], mockAddItem, - mockSetShowHelp, mockConfig, mockOnDebugMessage, mockHandleSlashCommand, @@ -1445,7 +1430,6 @@ describe('useGeminiStream', () => { new MockedGeminiClientClass(mockConfig), [], mockAddItem, - mockSetShowHelp, mockConfig, mockOnDebugMessage, mockHandleSlashCommand, @@ -1500,7 +1484,6 @@ describe('useGeminiStream', () => { new MockedGeminiClientClass(mockConfig), [], mockAddItem, - mockSetShowHelp, mockConfig, mockOnDebugMessage, mockHandleSlashCommand, @@ -1577,7 +1560,6 @@ describe('useGeminiStream', () => { new MockedGeminiClientClass(mockConfig), [], mockAddItem, - mockSetShowHelp, mockConfig, mockOnDebugMessage, mockHandleSlashCommand, @@ -1630,7 +1612,6 @@ describe('useGeminiStream', () => { new MockedGeminiClientClass(mockConfig), [], mockAddItem, - mockSetShowHelp, mockConfig, mockOnDebugMessage, mockHandleSlashCommand, diff --git a/packages/cli/src/ui/hooks/useGeminiStream.ts b/packages/cli/src/ui/hooks/useGeminiStream.ts index c934a139..e53e77dc 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.ts +++ b/packages/cli/src/ui/hooks/useGeminiStream.ts @@ -82,7 +82,6 @@ export const useGeminiStream = ( geminiClient: GeminiClient, history: HistoryItem[], addItem: UseHistoryManagerReturn['addItem'], - setShowHelp: React.Dispatch>, config: Config, onDebugMessage: (message: string) => void, handleSlashCommand: ( @@ -610,7 +609,6 @@ export const useGeminiStream = ( return; const userMessageTimestamp = Date.now(); - setShowHelp(false); // Reset quota error flag when starting a new query (not a continuation) if (!options?.isContinuation) { @@ -693,7 +691,6 @@ export const useGeminiStream = ( }, [ streamingState, - setShowHelp, setModelSwitchedFromQuotaError, prepareQueryForGemini, processGeminiStreamEvents, diff --git a/packages/cli/src/ui/types.ts b/packages/cli/src/ui/types.ts index da95d6ec..6d078b22 100644 --- a/packages/cli/src/ui/types.ts +++ b/packages/cli/src/ui/types.ts @@ -97,6 +97,11 @@ export type HistoryItemAbout = HistoryItemBase & { gcpProject: string; }; +export type HistoryItemHelp = HistoryItemBase & { + type: 'help'; + timestamp: Date; +}; + export type HistoryItemStats = HistoryItemBase & { type: 'stats'; duration: string; @@ -142,6 +147,7 @@ export type HistoryItemWithoutId = | HistoryItemInfo | HistoryItemError | HistoryItemAbout + | HistoryItemHelp | HistoryItemToolGroup | HistoryItemStats | HistoryItemModelStats @@ -157,6 +163,7 @@ export enum MessageType { ERROR = 'error', USER = 'user', ABOUT = 'about', + HELP = 'help', STATS = 'stats', MODEL_STATS = 'model_stats', TOOL_STATS = 'tool_stats', @@ -183,6 +190,11 @@ export type Message = gcpProject: string; content?: string; // Optional content, not really used for ABOUT } + | { + type: MessageType.HELP; + timestamp: Date; + content?: string; // Optional content, not really used for HELP + } | { type: MessageType.STATS; timestamp: Date;