From 61dbab03e0d512a681d32b0db432c585b9530fef Mon Sep 17 00:00:00 2001 From: Abhi <43648792+abhipatel12@users.noreply.github.com> Date: Tue, 6 Jan 2026 15:52:12 -0500 Subject: [PATCH] feat(ui): add visual indicators for hook execution (#15408) --- docs/get-started/configuration.md | 4 + .../cli/src/config/settingsSchema.test.ts | 9 + packages/cli/src/config/settingsSchema.ts | 9 + packages/cli/src/ui/AppContainer.test.tsx | 19 ++ packages/cli/src/ui/AppContainer.tsx | 11 +- .../cli/src/ui/components/Composer.test.tsx | 16 ++ packages/cli/src/ui/components/Composer.tsx | 40 +-- .../components/ContextSummaryDisplay.test.tsx | 85 ++++--- .../ui/components/HookStatusDisplay.test.tsx | 55 ++++ .../src/ui/components/HookStatusDisplay.tsx | 39 +++ .../src/ui/components/StatusDisplay.test.tsx | 208 ++++++++++++++++ .../cli/src/ui/components/StatusDisplay.tsx | 78 ++++++ .../ContextSummaryDisplay.test.tsx.snap | 12 + .../HookStatusDisplay.test.tsx.snap | 7 + .../__snapshots__/StatusDisplay.test.tsx.snap | 21 ++ packages/cli/src/ui/constants.ts | 3 + .../cli/src/ui/contexts/UIStateContext.tsx | 2 + .../src/ui/hooks/useHookDisplayState.test.ts | 234 ++++++++++++++++++ .../cli/src/ui/hooks/useHookDisplayState.ts | 104 ++++++++ packages/cli/src/ui/types.ts | 7 + .../core/src/hooks/hookEventHandler.test.ts | 37 +++ packages/core/src/hooks/hookEventHandler.ts | 31 ++- packages/core/src/hooks/hookRunner.test.ts | 62 +++++ packages/core/src/hooks/hookRunner.ts | 18 +- packages/core/src/utils/events.test.ts | 31 +++ packages/core/src/utils/events.ts | 48 ++++ schemas/settings.schema.json | 7 + 27 files changed, 1124 insertions(+), 73 deletions(-) create mode 100644 packages/cli/src/ui/components/HookStatusDisplay.test.tsx create mode 100644 packages/cli/src/ui/components/HookStatusDisplay.tsx create mode 100644 packages/cli/src/ui/components/StatusDisplay.test.tsx create mode 100644 packages/cli/src/ui/components/StatusDisplay.tsx create mode 100644 packages/cli/src/ui/components/__snapshots__/ContextSummaryDisplay.test.tsx.snap create mode 100644 packages/cli/src/ui/components/__snapshots__/HookStatusDisplay.test.tsx.snap create mode 100644 packages/cli/src/ui/components/__snapshots__/StatusDisplay.test.tsx.snap create mode 100644 packages/cli/src/ui/hooks/useHookDisplayState.test.ts create mode 100644 packages/cli/src/ui/hooks/useHookDisplayState.ts diff --git a/docs/get-started/configuration.md b/docs/get-started/configuration.md index 4ac0ac0764..58483baed1 100644 --- a/docs/get-started/configuration.md +++ b/docs/get-started/configuration.md @@ -872,6 +872,10 @@ their corresponding top-level category object in your `settings.json` file. Hooks in this list will not execute even if configured. - **Default:** `[]` +- **`hooks.notifications`** (boolean): + - **Description:** Show visual indicators when hooks are executing. + - **Default:** `true` + - **`hooks.BeforeTool`** (array): - **Description:** Hooks that execute before tool execution. Can intercept, validate, or modify tool calls. diff --git a/packages/cli/src/config/settingsSchema.test.ts b/packages/cli/src/config/settingsSchema.test.ts index 81f57feefe..e5706e0d6f 100644 --- a/packages/cli/src/config/settingsSchema.test.ts +++ b/packages/cli/src/config/settingsSchema.test.ts @@ -357,6 +357,15 @@ describe('SettingsSchema', () => { ); }); + it('should have hooks.notifications setting in schema', () => { + const setting = getSettingsSchema().hooks.properties.notifications; + expect(setting).toBeDefined(); + expect(setting.type).toBe('boolean'); + expect(setting.category).toBe('Advanced'); + expect(setting.default).toBe(true); + expect(setting.showInDialog).toBe(true); + }); + it('should have name and description in hook definitions', () => { const hookDef = SETTINGS_SCHEMA_DEFINITIONS['HookDefinitionArray']; expect(hookDef).toBeDefined(); diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index a2d7b2a008..08fb30a3cf 100644 --- a/packages/cli/src/config/settingsSchema.ts +++ b/packages/cli/src/config/settingsSchema.ts @@ -1559,6 +1559,15 @@ const SETTINGS_SCHEMA = { }, mergeStrategy: MergeStrategy.UNION, }, + notifications: { + type: 'boolean', + label: 'Hook Notifications', + category: 'Advanced', + requiresRestart: false, + default: true, + description: 'Show visual indicators when hooks are executing.', + showInDialog: true, + }, BeforeTool: { type: 'array', label: 'Before Tool Hooks', diff --git a/packages/cli/src/ui/AppContainer.test.tsx b/packages/cli/src/ui/AppContainer.test.tsx index 1d7fce5aa9..939045d44a 100644 --- a/packages/cli/src/ui/AppContainer.test.tsx +++ b/packages/cli/src/ui/AppContainer.test.tsx @@ -143,6 +143,7 @@ vi.mock('./contexts/SessionContext.js'); vi.mock('./components/shared/text-buffer.js'); vi.mock('./hooks/useLogger.js'); vi.mock('./hooks/useInputHistoryStore.js'); +vi.mock('./hooks/useHookDisplayState.js'); // Mock external utilities vi.mock('../utils/events.js'); @@ -171,6 +172,7 @@ import { useTextBuffer } from './components/shared/text-buffer.js'; import { useLogger } from './hooks/useLogger.js'; import { useLoadingIndicator } from './hooks/useLoadingIndicator.js'; import { useInputHistoryStore } from './hooks/useInputHistoryStore.js'; +import { useHookDisplayState } from './hooks/useHookDisplayState.js'; import { useKeypress, type Key } from './hooks/useKeypress.js'; import { measureElement } from 'ink'; import { useTerminalSize } from './hooks/useTerminalSize.js'; @@ -243,6 +245,7 @@ describe('AppContainer State Management', () => { const mockedUseLoadingIndicator = useLoadingIndicator as Mock; const mockedUseKeypress = useKeypress as Mock; const mockedUseInputHistoryStore = useInputHistoryStore as Mock; + const mockedUseHookDisplayState = useHookDisplayState as Mock; beforeEach(() => { vi.clearAllMocks(); @@ -363,6 +366,7 @@ describe('AppContainer State Management', () => { elapsedTime: '0.0s', currentLoadingPhrase: '', }); + mockedUseHookDisplayState.mockReturnValue([]); // Mock Config mockConfig = makeFakeConfig(); @@ -1874,6 +1878,21 @@ describe('AppContainer State Management', () => { expect(capturedUIState.currentModel).toBe('new-model'); unmount!(); }); + + it('provides activeHooks from useHookDisplayState', async () => { + const mockHooks = [{ name: 'hook1', eventName: 'event1' }]; + mockedUseHookDisplayState.mockReturnValue(mockHooks); + + let unmount: () => void; + await act(async () => { + const result = renderAppContainer(); + unmount = result.unmount; + }); + await waitFor(() => expect(capturedUIState).toBeTruthy()); + + expect(capturedUIState.activeHooks).toEqual(mockHooks); + unmount!(); + }); }); describe('Shell Interaction', () => { diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index f352556b06..7687a096c1 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -123,9 +123,11 @@ import { useSettings } from './contexts/SettingsContext.js'; import { terminalCapabilityManager } from './utils/terminalCapabilityManager.js'; import { useInputHistoryStore } from './hooks/useInputHistoryStore.js'; import { useBanner } from './hooks/useBanner.js'; - -const WARNING_PROMPT_DURATION_MS = 1000; -const QUEUE_ERROR_DISPLAY_DURATION_MS = 3000; +import { useHookDisplayState } from './hooks/useHookDisplayState.js'; +import { + WARNING_PROMPT_DURATION_MS, + QUEUE_ERROR_DISPLAY_DURATION_MS, +} from './constants.js'; function isToolExecuting(pendingHistoryItems: HistoryItemWithoutId[]) { return pendingHistoryItems.some((item) => { @@ -189,6 +191,7 @@ export const AppContainer = (props: AppContainerProps) => { useState(false); const [historyRemountKey, setHistoryRemountKey] = useState(0); const [settingsNonce, setSettingsNonce] = useState(0); + const activeHooks = useHookDisplayState(); const [updateInfo, setUpdateInfo] = useState(null); const [isTrustedFolder, setIsTrustedFolder] = useState( isWorkspaceTrusted(settings.merged).isTrusted, @@ -1522,6 +1525,7 @@ Logging in with Google... Restarting Gemini CLI to continue. elapsedTime, currentLoadingPhrase, historyRemountKey, + activeHooks, messageQueue, queueErrorMessage, showAutoAcceptIndicator, @@ -1612,6 +1616,7 @@ Logging in with Google... Restarting Gemini CLI to continue. elapsedTime, currentLoadingPhrase, historyRemountKey, + activeHooks, messageQueue, queueErrorMessage, showAutoAcceptIndicator, diff --git a/packages/cli/src/ui/components/Composer.test.tsx b/packages/cli/src/ui/components/Composer.test.tsx index ef97c56201..b48e18cc00 100644 --- a/packages/cli/src/ui/components/Composer.test.tsx +++ b/packages/cli/src/ui/components/Composer.test.tsx @@ -36,6 +36,10 @@ vi.mock('./ContextSummaryDisplay.js', () => ({ ContextSummaryDisplay: () => ContextSummaryDisplay, })); +vi.mock('./HookStatusDisplay.js', () => ({ + HookStatusDisplay: () => HookStatusDisplay, +})); + vi.mock('./AutoAcceptIndicator.js', () => ({ AutoAcceptIndicator: () => AutoAcceptIndicator, })); @@ -125,6 +129,7 @@ const createMockUIState = (overrides: Partial = {}): UIState => errorCount: 0, nightly: false, isTrustedFolder: true, + activeHooks: [], ...overrides, }) as UIState; @@ -341,6 +346,17 @@ describe('Composer', () => { expect(lastFrame()).toContain('ContextSummaryDisplay'); }); + it('renders HookStatusDisplay instead of ContextSummaryDisplay with active hooks', () => { + const uiState = createMockUIState({ + activeHooks: [{ name: 'test-hook', eventName: 'before-agent' }], + }); + + const { lastFrame } = renderComposer(uiState); + + expect(lastFrame()).toContain('HookStatusDisplay'); + expect(lastFrame()).not.toContain('ContextSummaryDisplay'); + }); + it('shows Ctrl+C exit prompt when ctrlCPressedOnce is true', () => { const uiState = createMockUIState({ ctrlCPressedOnce: true, diff --git a/packages/cli/src/ui/components/Composer.tsx b/packages/cli/src/ui/components/Composer.tsx index 11685a4435..d48cced332 100644 --- a/packages/cli/src/ui/components/Composer.tsx +++ b/packages/cli/src/ui/components/Composer.tsx @@ -5,9 +5,9 @@ */ import { useState } from 'react'; -import { Box, Text, useIsScreenReaderEnabled } from 'ink'; +import { Box, useIsScreenReaderEnabled } from 'ink'; import { LoadingIndicator } from './LoadingIndicator.js'; -import { ContextSummaryDisplay } from './ContextSummaryDisplay.js'; +import { StatusDisplay } from './StatusDisplay.js'; import { AutoAcceptIndicator } from './AutoAcceptIndicator.js'; import { ShellModeIndicator } from './ShellModeIndicator.js'; import { DetailedMessagesDisplay } from './DetailedMessagesDisplay.js'; @@ -17,7 +17,6 @@ import { Footer } from './Footer.js'; import { ShowMoreLines } from './ShowMoreLines.js'; import { QueuedMessageDisplay } from './QueuedMessageDisplay.js'; import { OverflowProvider } from '../contexts/OverflowContext.js'; -import { theme } from '../semantic-colors.js'; import { isNarrowWidth } from '../utils/isNarrowWidth.js'; import { useUIState } from '../contexts/UIStateContext.js'; import { useUIActions } from '../contexts/UIActionsContext.js'; @@ -43,7 +42,7 @@ export const Composer = () => { const [suggestionsVisible, setSuggestionsVisible] = useState(false); const isAlternateBuffer = useAlternateBuffer(); - const { contextFileNames, showAutoAcceptIndicator } = uiState; + const { showAutoAcceptIndicator } = uiState; const suggestionsPosition = isAlternateBuffer ? 'above' : 'below'; const hideContextSummary = suggestionsVisible && suggestionsPosition === 'above'; @@ -92,38 +91,7 @@ export const Composer = () => { alignItems={isNarrow ? 'flex-start' : 'center'} > - {process.env['GEMINI_SYSTEM_MD'] && ( - |⌐■_■| - )} - {uiState.ctrlCPressedOnce ? ( - - Press Ctrl+C again to exit. - - ) : uiState.warningMessage ? ( - {uiState.warningMessage} - ) : uiState.ctrlDPressedOnce ? ( - - Press Ctrl+D again to exit. - - ) : uiState.showEscapePrompt ? ( - Press Esc again to clear. - ) : uiState.queueErrorMessage ? ( - {uiState.queueErrorMessage} - ) : ( - !settings.merged.ui?.hideContextSummary && - !hideContextSummary && ( - - ) - )} + {showAutoAcceptIndicator !== ApprovalMode.DEFAULT && diff --git a/packages/cli/src/ui/components/ContextSummaryDisplay.test.tsx b/packages/cli/src/ui/components/ContextSummaryDisplay.test.tsx index 50a610f345..415e867a87 100644 --- a/packages/cli/src/ui/components/ContextSummaryDisplay.test.tsx +++ b/packages/cli/src/ui/components/ContextSummaryDisplay.test.tsx @@ -6,7 +6,7 @@ import type React from 'react'; import { render } from '../../test-utils/render.js'; -import { describe, it, expect, vi } from 'vitest'; +import { describe, it, expect, vi, afterEach } from 'vitest'; import { ContextSummaryDisplay } from './ContextSummaryDisplay.js'; import * as useTerminalSize from '../hooks/useTerminalSize.js'; @@ -16,6 +16,11 @@ vi.mock('../hooks/useTerminalSize.js', () => ({ const useTerminalSizeMock = vi.mocked(useTerminalSize.useTerminalSize); +afterEach(() => { + vi.restoreAllMocks(); + vi.useRealTimers(); +}); + const renderWithWidth = ( width: number, props: React.ComponentProps, @@ -26,48 +31,68 @@ const renderWithWidth = ( describe('', () => { const baseProps = { - geminiMdFileCount: 1, - contextFileNames: ['GEMINI.md'], - mcpServers: { 'test-server': { command: 'test' } }, + geminiMdFileCount: 0, + contextFileNames: [], + mcpServers: {}, ideContext: { workspaceState: { - openFiles: [{ path: '/a/b/c', timestamp: Date.now() }], + openFiles: [], }, }, skillCount: 1, }; it('should render on a single line on a wide screen', () => { - const { lastFrame, unmount } = renderWithWidth(120, baseProps); - const output = lastFrame()!; - expect(output).toContain( - '1 open file (ctrl+g to view) | 1 GEMINI.md file | 1 MCP server | 1 skill', - ); - expect(output).not.toContain('Using:'); - // Check for absence of newlines - expect(output.includes('\n')).toBe(false); + const props = { + ...baseProps, + geminiMdFileCount: 1, + contextFileNames: ['GEMINI.md'], + mcpServers: { 'test-server': { command: 'test' } }, + ideContext: { + workspaceState: { + openFiles: [{ path: '/a/b/c', timestamp: Date.now() }], + }, + }, + }; + const { lastFrame, unmount } = renderWithWidth(120, props); + expect(lastFrame()).toMatchSnapshot(); unmount(); }); it('should render on multiple lines on a narrow screen', () => { - const { lastFrame, unmount } = renderWithWidth(60, baseProps); - const output = lastFrame()!; - const expectedLines = [ - ' - 1 open file (ctrl+g to view)', - ' - 1 GEMINI.md file', - ' - 1 MCP server', - ' - 1 skill', - ]; - const actualLines = output.split('\n'); - expect(actualLines).toEqual(expectedLines); + const props = { + ...baseProps, + geminiMdFileCount: 1, + contextFileNames: ['GEMINI.md'], + mcpServers: { 'test-server': { command: 'test' } }, + ideContext: { + workspaceState: { + openFiles: [{ path: '/a/b/c', timestamp: Date.now() }], + }, + }, + }; + const { lastFrame, unmount } = renderWithWidth(60, props); + expect(lastFrame()).toMatchSnapshot(); unmount(); }); it('should switch layout at the 80-column breakpoint', () => { + const props = { + ...baseProps, + geminiMdFileCount: 1, + contextFileNames: ['GEMINI.md'], + mcpServers: { 'test-server': { command: 'test' } }, + ideContext: { + workspaceState: { + openFiles: [{ path: '/a/b/c', timestamp: Date.now() }], + }, + }, + }; + // At 80 columns, should be on one line const { lastFrame: wideFrame, unmount: unmountWide } = renderWithWidth( 80, - baseProps, + props, ); expect(wideFrame()!.includes('\n')).toBe(false); unmountWide(); @@ -75,13 +100,12 @@ describe('', () => { // At 79 columns, should be on multiple lines const { lastFrame: narrowFrame, unmount: unmountNarrow } = renderWithWidth( 79, - baseProps, + props, ); expect(narrowFrame()!.includes('\n')).toBe(true); expect(narrowFrame()!.split('\n').length).toBe(4); unmountNarrow(); }); - it('should not render empty parts', () => { const props = { ...baseProps, @@ -89,11 +113,14 @@ describe('', () => { contextFileNames: [], mcpServers: {}, skillCount: 0, + ideContext: { + workspaceState: { + openFiles: [{ path: '/a/b/c', timestamp: Date.now() }], + }, + }, }; const { lastFrame, unmount } = renderWithWidth(60, props); - const expectedLines = [' - 1 open file (ctrl+g to view)']; - const actualLines = lastFrame()!.split('\n'); - expect(actualLines).toEqual(expectedLines); + expect(lastFrame()).toMatchSnapshot(); unmount(); }); }); diff --git a/packages/cli/src/ui/components/HookStatusDisplay.test.tsx b/packages/cli/src/ui/components/HookStatusDisplay.test.tsx new file mode 100644 index 0000000000..69aa2cdb25 --- /dev/null +++ b/packages/cli/src/ui/components/HookStatusDisplay.test.tsx @@ -0,0 +1,55 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { render } from '../../test-utils/render.js'; +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { HookStatusDisplay } from './HookStatusDisplay.js'; + +afterEach(() => { + vi.restoreAllMocks(); + vi.useRealTimers(); +}); + +describe('', () => { + it('should render a single executing hook', () => { + const props = { + activeHooks: [{ name: 'test-hook', eventName: 'BeforeAgent' }], + }; + const { lastFrame, unmount } = render(); + expect(lastFrame()).toMatchSnapshot(); + unmount(); + }); + + it('should render multiple executing hooks', () => { + const props = { + activeHooks: [ + { name: 'h1', eventName: 'BeforeAgent' }, + { name: 'h2', eventName: 'BeforeAgent' }, + ], + }; + const { lastFrame, unmount } = render(); + expect(lastFrame()).toMatchSnapshot(); + unmount(); + }); + + it('should render sequential hook progress', () => { + const props = { + activeHooks: [ + { name: 'step', eventName: 'BeforeAgent', index: 1, total: 3 }, + ], + }; + const { lastFrame, unmount } = render(); + expect(lastFrame()).toMatchSnapshot(); + unmount(); + }); + + it('should return empty string if no active hooks', () => { + const props = { activeHooks: [] }; + const { lastFrame, unmount } = render(); + expect(lastFrame()).toBe(''); + unmount(); + }); +}); diff --git a/packages/cli/src/ui/components/HookStatusDisplay.tsx b/packages/cli/src/ui/components/HookStatusDisplay.tsx new file mode 100644 index 0000000000..07b2ee3d4a --- /dev/null +++ b/packages/cli/src/ui/components/HookStatusDisplay.tsx @@ -0,0 +1,39 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import type React from 'react'; +import { Text } from 'ink'; +import { theme } from '../semantic-colors.js'; +import { type ActiveHook } from '../types.js'; + +interface HookStatusDisplayProps { + activeHooks: ActiveHook[]; +} + +export const HookStatusDisplay: React.FC = ({ + activeHooks, +}) => { + if (activeHooks.length === 0) { + return null; + } + + const label = activeHooks.length > 1 ? 'Executing Hooks' : 'Executing Hook'; + const displayNames = activeHooks.map((hook) => { + let name = hook.name; + if (hook.index && hook.total && hook.total > 1) { + name += ` (${hook.index}/${hook.total})`; + } + return name; + }); + + const text = `${label}: ${displayNames.join(', ')}`; + + return ( + + {text} + + ); +}; diff --git a/packages/cli/src/ui/components/StatusDisplay.test.tsx b/packages/cli/src/ui/components/StatusDisplay.test.tsx new file mode 100644 index 0000000000..e5a64bd3f3 --- /dev/null +++ b/packages/cli/src/ui/components/StatusDisplay.test.tsx @@ -0,0 +1,208 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render } from '../../test-utils/render.js'; +import { Text } from 'ink'; +import { StatusDisplay } from './StatusDisplay.js'; +import { UIStateContext, type UIState } from '../contexts/UIStateContext.js'; +import { ConfigContext } from '../contexts/ConfigContext.js'; +import { SettingsContext } from '../contexts/SettingsContext.js'; + +// Mock child components to simplify testing +vi.mock('./ContextSummaryDisplay.js', () => ({ + ContextSummaryDisplay: (props: { skillCount: number }) => ( + Mock Context Summary Display (Skills: {props.skillCount}) + ), +})); + +vi.mock('./HookStatusDisplay.js', () => ({ + HookStatusDisplay: () => Mock Hook Status Display, +})); + +// Create mock context providers +const createMockUIState = (overrides: Partial = {}): UIState => + ({ + ctrlCPressedOnce: false, + warningMessage: null, + ctrlDPressedOnce: false, + showEscapePrompt: false, + queueErrorMessage: null, + activeHooks: [], + ideContextState: null, + geminiMdFileCount: 0, + contextFileNames: [], + ...overrides, + }) as UIState; + +const createMockConfig = (overrides = {}) => ({ + getMcpClientManager: vi.fn().mockImplementation(() => ({ + getBlockedMcpServers: vi.fn(() => []), + getMcpServers: vi.fn(() => ({})), + })), + getSkillManager: vi.fn().mockImplementation(() => ({ + getSkills: vi.fn(() => ['skill1', 'skill2']), + })), + ...overrides, +}); + +const createMockSettings = (merged = {}) => ({ + merged: { + hooks: { notifications: true }, + ui: { hideContextSummary: false }, + ...merged, + }, +}); + +/* eslint-disable @typescript-eslint/no-explicit-any */ +const renderStatusDisplay = ( + props: { hideContextSummary: boolean } = { hideContextSummary: false }, + uiState: UIState = createMockUIState(), + settings = createMockSettings(), + config = createMockConfig(), +) => + render( + + + + + + + , + ); +/* eslint-enable @typescript-eslint/no-explicit-any */ + +describe('StatusDisplay', () => { + const originalEnv = process.env; + + afterEach(() => { + process.env = { ...originalEnv }; + delete process.env['GEMINI_SYSTEM_MD']; + }); + + it('renders nothing by default if context summary is hidden via props', () => { + const { lastFrame } = renderStatusDisplay({ hideContextSummary: true }); + expect(lastFrame()).toBe(''); + }); + + it('renders ContextSummaryDisplay by default', () => { + const { lastFrame } = renderStatusDisplay(); + expect(lastFrame()).toMatchSnapshot(); + }); + + it('renders system md indicator if env var is set', () => { + process.env['GEMINI_SYSTEM_MD'] = 'true'; + const { lastFrame } = renderStatusDisplay(); + expect(lastFrame()).toMatchSnapshot(); + }); + + it('prioritizes Ctrl+C prompt over everything else (except system md)', () => { + const uiState = createMockUIState({ + ctrlCPressedOnce: true, + warningMessage: 'Warning', + activeHooks: [{ name: 'hook', eventName: 'event' }], + }); + const { lastFrame } = renderStatusDisplay( + { hideContextSummary: false }, + uiState, + ); + expect(lastFrame()).toMatchSnapshot(); + }); + + it('renders warning message', () => { + const uiState = createMockUIState({ + warningMessage: 'This is a warning', + }); + const { lastFrame } = renderStatusDisplay( + { hideContextSummary: false }, + uiState, + ); + expect(lastFrame()).toMatchSnapshot(); + }); + + it('prioritizes warning over Ctrl+D', () => { + const uiState = createMockUIState({ + warningMessage: 'Warning', + ctrlDPressedOnce: true, + }); + const { lastFrame } = renderStatusDisplay( + { hideContextSummary: false }, + uiState, + ); + expect(lastFrame()).toMatchSnapshot(); + }); + + it('renders Ctrl+D prompt', () => { + const uiState = createMockUIState({ + ctrlDPressedOnce: true, + }); + const { lastFrame } = renderStatusDisplay( + { hideContextSummary: false }, + uiState, + ); + expect(lastFrame()).toMatchSnapshot(); + }); + + it('renders Escape prompt', () => { + const uiState = createMockUIState({ + showEscapePrompt: true, + }); + const { lastFrame } = renderStatusDisplay( + { hideContextSummary: false }, + uiState, + ); + expect(lastFrame()).toMatchSnapshot(); + }); + + it('renders Queue Error Message', () => { + const uiState = createMockUIState({ + queueErrorMessage: 'Queue Error', + }); + const { lastFrame } = renderStatusDisplay( + { hideContextSummary: false }, + uiState, + ); + expect(lastFrame()).toMatchSnapshot(); + }); + + it('renders HookStatusDisplay when hooks are active', () => { + const uiState = createMockUIState({ + activeHooks: [{ name: 'hook', eventName: 'event' }], + }); + const { lastFrame } = renderStatusDisplay( + { hideContextSummary: false }, + uiState, + ); + expect(lastFrame()).toMatchSnapshot(); + }); + + it('does NOT render HookStatusDisplay if notifications are disabled in settings', () => { + const uiState = createMockUIState({ + activeHooks: [{ name: 'hook', eventName: 'event' }], + }); + const settings = createMockSettings({ + hooks: { notifications: false }, + }); + const { lastFrame } = renderStatusDisplay( + { hideContextSummary: false }, + uiState, + settings, + ); + expect(lastFrame()).toMatchSnapshot(); + }); + + it('hides ContextSummaryDisplay if configured in settings', () => { + const settings = createMockSettings({ + ui: { hideContextSummary: true }, + }); + const { lastFrame } = renderStatusDisplay( + { hideContextSummary: false }, + undefined, + settings, + ); + expect(lastFrame()).toBe(''); + }); +}); diff --git a/packages/cli/src/ui/components/StatusDisplay.tsx b/packages/cli/src/ui/components/StatusDisplay.tsx new file mode 100644 index 0000000000..367be17593 --- /dev/null +++ b/packages/cli/src/ui/components/StatusDisplay.tsx @@ -0,0 +1,78 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import type React from 'react'; +import { Text } from 'ink'; +import { theme } from '../semantic-colors.js'; +import { useUIState } from '../contexts/UIStateContext.js'; +import { useSettings } from '../contexts/SettingsContext.js'; +import { useConfig } from '../contexts/ConfigContext.js'; +import { ContextSummaryDisplay } from './ContextSummaryDisplay.js'; +import { HookStatusDisplay } from './HookStatusDisplay.js'; + +interface StatusDisplayProps { + hideContextSummary: boolean; +} + +export const StatusDisplay: React.FC = ({ + hideContextSummary, +}) => { + const uiState = useUIState(); + const settings = useSettings(); + const config = useConfig(); + + if (process.env['GEMINI_SYSTEM_MD']) { + return |⌐■_■| ; + } + + if (uiState.ctrlCPressedOnce) { + return ( + Press Ctrl+C again to exit. + ); + } + + if (uiState.warningMessage) { + return {uiState.warningMessage}; + } + + if (uiState.ctrlDPressedOnce) { + return ( + Press Ctrl+D again to exit. + ); + } + + if (uiState.showEscapePrompt) { + return Press Esc again to clear.; + } + + if (uiState.queueErrorMessage) { + return {uiState.queueErrorMessage}; + } + + if ( + uiState.activeHooks.length > 0 && + (settings.merged.hooks?.notifications ?? true) + ) { + return ; + } + + if (!settings.merged.ui?.hideContextSummary && !hideContextSummary) { + return ( + + ); + } + + return null; +}; diff --git a/packages/cli/src/ui/components/__snapshots__/ContextSummaryDisplay.test.tsx.snap b/packages/cli/src/ui/components/__snapshots__/ContextSummaryDisplay.test.tsx.snap new file mode 100644 index 0000000000..5146c01e39 --- /dev/null +++ b/packages/cli/src/ui/components/__snapshots__/ContextSummaryDisplay.test.tsx.snap @@ -0,0 +1,12 @@ +// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html + +exports[` > should not render empty parts 1`] = `" - 1 open file (ctrl+g to view)"`; + +exports[` > should render on a single line on a wide screen 1`] = `" 1 open file (ctrl+g to view) | 1 GEMINI.md file | 1 MCP server | 1 skill"`; + +exports[` > should render on multiple lines on a narrow screen 1`] = ` +" - 1 open file (ctrl+g to view) + - 1 GEMINI.md file + - 1 MCP server + - 1 skill" +`; diff --git a/packages/cli/src/ui/components/__snapshots__/HookStatusDisplay.test.tsx.snap b/packages/cli/src/ui/components/__snapshots__/HookStatusDisplay.test.tsx.snap new file mode 100644 index 0000000000..8d07035c07 --- /dev/null +++ b/packages/cli/src/ui/components/__snapshots__/HookStatusDisplay.test.tsx.snap @@ -0,0 +1,7 @@ +// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html + +exports[` > should render a single executing hook 1`] = `"Executing Hook: test-hook"`; + +exports[` > should render multiple executing hooks 1`] = `"Executing Hooks: h1, h2"`; + +exports[` > should render sequential hook progress 1`] = `"Executing Hook: step (1/3)"`; diff --git a/packages/cli/src/ui/components/__snapshots__/StatusDisplay.test.tsx.snap b/packages/cli/src/ui/components/__snapshots__/StatusDisplay.test.tsx.snap new file mode 100644 index 0000000000..ee2c68fbd5 --- /dev/null +++ b/packages/cli/src/ui/components/__snapshots__/StatusDisplay.test.tsx.snap @@ -0,0 +1,21 @@ +// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html + +exports[`StatusDisplay > does NOT render HookStatusDisplay if notifications are disabled in settings 1`] = `"Mock Context Summary Display (Skills: 2)"`; + +exports[`StatusDisplay > prioritizes Ctrl+C prompt over everything else (except system md) 1`] = `"Press Ctrl+C again to exit."`; + +exports[`StatusDisplay > prioritizes warning over Ctrl+D 1`] = `"Warning"`; + +exports[`StatusDisplay > renders ContextSummaryDisplay by default 1`] = `"Mock Context Summary Display (Skills: 2)"`; + +exports[`StatusDisplay > renders Ctrl+D prompt 1`] = `"Press Ctrl+D again to exit."`; + +exports[`StatusDisplay > renders Escape prompt 1`] = `"Press Esc again to clear."`; + +exports[`StatusDisplay > renders HookStatusDisplay when hooks are active 1`] = `"Mock Hook Status Display"`; + +exports[`StatusDisplay > renders Queue Error Message 1`] = `"Queue Error"`; + +exports[`StatusDisplay > renders system md indicator if env var is set 1`] = `"|⌐■_■|"`; + +exports[`StatusDisplay > renders warning message 1`] = `"This is a warning"`; diff --git a/packages/cli/src/ui/constants.ts b/packages/cli/src/ui/constants.ts index 9bd4cf1b34..e27f3c3bbe 100644 --- a/packages/cli/src/ui/constants.ts +++ b/packages/cli/src/ui/constants.ts @@ -28,3 +28,6 @@ export const TOOL_STATUS = { // Maximum number of MCP resources to display per server before truncating export const MAX_MCP_RESOURCES_TO_SHOW = 10; + +export const WARNING_PROMPT_DURATION_MS = 1000; +export const QUEUE_ERROR_DISPLAY_DURATION_MS = 3000; diff --git a/packages/cli/src/ui/contexts/UIStateContext.tsx b/packages/cli/src/ui/contexts/UIStateContext.tsx index d9f74e59e0..1175b0743a 100644 --- a/packages/cli/src/ui/contexts/UIStateContext.tsx +++ b/packages/cli/src/ui/contexts/UIStateContext.tsx @@ -14,6 +14,7 @@ import type { LoopDetectionConfirmationRequest, HistoryItemWithoutId, StreamingState, + ActiveHook, } from '../types.js'; import type { CommandContext, SlashCommand } from '../commands/types.js'; import type { TextBuffer } from '../components/shared/text-buffer.js'; @@ -96,6 +97,7 @@ export interface UIState { elapsedTime: number; currentLoadingPhrase: string; historyRemountKey: number; + activeHooks: ActiveHook[]; messageQueue: string[]; queueErrorMessage: string | null; showAutoAcceptIndicator: ApprovalMode; diff --git a/packages/cli/src/ui/hooks/useHookDisplayState.test.ts b/packages/cli/src/ui/hooks/useHookDisplayState.test.ts new file mode 100644 index 0000000000..a6c86401bd --- /dev/null +++ b/packages/cli/src/ui/hooks/useHookDisplayState.test.ts @@ -0,0 +1,234 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { renderHook } from '../../test-utils/render.js'; +import { describe, it, expect, vi, afterEach, beforeEach } from 'vitest'; +import { useHookDisplayState } from './useHookDisplayState.js'; +import { + coreEvents, + CoreEvent, + type HookStartPayload, + type HookEndPayload, +} from '@google/gemini-cli-core'; +import { act } from 'react'; + +describe('useHookDisplayState', () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + vi.useRealTimers(); + coreEvents.removeAllListeners(CoreEvent.HookStart); + coreEvents.removeAllListeners(CoreEvent.HookEnd); + }); + + it('should initialize with empty hooks', () => { + const { result } = renderHook(() => useHookDisplayState()); + expect(result.current).toEqual([]); + }); + + it('should add a hook when HookStart event is emitted', () => { + const { result } = renderHook(() => useHookDisplayState()); + + const payload: HookStartPayload = { + hookName: 'test-hook', + eventName: 'before-agent', + hookIndex: 1, + totalHooks: 1, + }; + + act(() => { + coreEvents.emitHookStart(payload); + }); + + expect(result.current).toHaveLength(1); + expect(result.current[0]).toMatchObject({ + name: 'test-hook', + eventName: 'before-agent', + }); + }); + + it('should remove a hook immediately if duration > 1s', () => { + const { result } = renderHook(() => useHookDisplayState()); + + const startPayload: HookStartPayload = { + hookName: 'test-hook', + eventName: 'before-agent', + }; + + act(() => { + coreEvents.emitHookStart(startPayload); + }); + + // Advance time by 1.1 seconds + act(() => { + vi.advanceTimersByTime(1100); + }); + + const endPayload: HookEndPayload = { + hookName: 'test-hook', + eventName: 'before-agent', + success: true, + }; + + act(() => { + coreEvents.emitHookEnd(endPayload); + }); + + expect(result.current).toHaveLength(0); + }); + + it('should delay removal if duration < 1s', () => { + const { result } = renderHook(() => useHookDisplayState()); + + const startPayload: HookStartPayload = { + hookName: 'test-hook', + eventName: 'before-agent', + }; + + act(() => { + coreEvents.emitHookStart(startPayload); + }); + + // Advance time by only 100ms + act(() => { + vi.advanceTimersByTime(100); + }); + + const endPayload: HookEndPayload = { + hookName: 'test-hook', + eventName: 'before-agent', + success: true, + }; + + act(() => { + coreEvents.emitHookEnd(endPayload); + }); + + // Should still be present + expect(result.current).toHaveLength(1); + + // Advance remaining time (900ms needed, let's go 950ms) + act(() => { + vi.advanceTimersByTime(950); + }); + + expect(result.current).toHaveLength(0); + }); + + it('should handle multiple hooks correctly', () => { + const { result } = renderHook(() => useHookDisplayState()); + + act(() => { + coreEvents.emitHookStart({ hookName: 'h1', eventName: 'e1' }); + }); + + act(() => { + vi.advanceTimersByTime(500); + }); + + act(() => { + coreEvents.emitHookStart({ hookName: 'h2', eventName: 'e1' }); + }); + + expect(result.current).toHaveLength(2); + + // End h1 (total time 500ms -> needs 500ms delay) + act(() => { + coreEvents.emitHookEnd({ + hookName: 'h1', + eventName: 'e1', + success: true, + }); + }); + + // h1 still there + expect(result.current).toHaveLength(2); + + // Advance 600ms. h1 should disappear. h2 has been running for 600ms. + act(() => { + vi.advanceTimersByTime(600); + }); + + expect(result.current).toHaveLength(1); + expect(result.current[0].name).toBe('h2'); + + // End h2 (total time 600ms -> needs 400ms delay) + act(() => { + coreEvents.emitHookEnd({ + hookName: 'h2', + eventName: 'e1', + success: true, + }); + }); + + expect(result.current).toHaveLength(1); + + act(() => { + vi.advanceTimersByTime(500); + }); + + expect(result.current).toHaveLength(0); + }); + + it('should handle interleaved hooks with same name and event', () => { + const { result } = renderHook(() => useHookDisplayState()); + const hook = { hookName: 'same-hook', eventName: 'same-event' }; + + // Start Hook 1 at t=0 + act(() => { + coreEvents.emitHookStart(hook); + }); + + // Advance to t=500 + act(() => { + vi.advanceTimersByTime(500); + }); + + // Start Hook 2 at t=500 + act(() => { + coreEvents.emitHookStart(hook); + }); + + expect(result.current).toHaveLength(2); + expect(result.current[0].name).toBe('same-hook'); + expect(result.current[1].name).toBe('same-hook'); + + // End Hook 1 at t=600 (Duration 600ms -> delay 400ms) + act(() => { + vi.advanceTimersByTime(100); + coreEvents.emitHookEnd({ ...hook, success: true }); + }); + + // Both still visible (Hook 1 pending removal in 400ms) + expect(result.current).toHaveLength(2); + + // Advance 400ms (t=1000). Hook 1 should be removed. + act(() => { + vi.advanceTimersByTime(400); + }); + + expect(result.current).toHaveLength(1); + + // End Hook 2 at t=1100 (Duration: 1100 - 500 = 600ms -> delay 400ms) + act(() => { + vi.advanceTimersByTime(100); + coreEvents.emitHookEnd({ ...hook, success: true }); + }); + + // Hook 2 still visible (pending removal in 400ms) + expect(result.current).toHaveLength(1); + + // Advance 400ms (t=1500). Hook 2 should be removed. + act(() => { + vi.advanceTimersByTime(400); + }); + + expect(result.current).toHaveLength(0); + }); +}); diff --git a/packages/cli/src/ui/hooks/useHookDisplayState.ts b/packages/cli/src/ui/hooks/useHookDisplayState.ts new file mode 100644 index 0000000000..6c9e1811ad --- /dev/null +++ b/packages/cli/src/ui/hooks/useHookDisplayState.ts @@ -0,0 +1,104 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { useState, useEffect, useRef } from 'react'; +import { + coreEvents, + CoreEvent, + type HookStartPayload, + type HookEndPayload, +} from '@google/gemini-cli-core'; +import { type ActiveHook } from '../types.js'; +import { WARNING_PROMPT_DURATION_MS } from '../constants.js'; + +export const useHookDisplayState = () => { + const [activeHooks, setActiveHooks] = useState([]); + + // Track start times independently of render state to calculate duration in event handlers + // Key: `${hookName}:${eventName}` -> Stack of StartTimes (FIFO) + const hookStartTimes = useRef>(new Map()); + + // Track active timeouts to clear them on unmount + const timeouts = useRef>(new Set()); + + useEffect(() => { + const activeTimeouts = timeouts.current; + const startTimes = hookStartTimes.current; + + const handleHookStart = (payload: HookStartPayload) => { + const key = `${payload.hookName}:${payload.eventName}`; + const now = Date.now(); + + // Add start time to ref + if (!startTimes.has(key)) { + startTimes.set(key, []); + } + startTimes.get(key)!.push(now); + + setActiveHooks((prev) => [ + ...prev, + { + name: payload.hookName, + eventName: payload.eventName, + index: payload.hookIndex, + total: payload.totalHooks, + }, + ]); + }; + + const handleHookEnd = (payload: HookEndPayload) => { + const key = `${payload.hookName}:${payload.eventName}`; + const starts = startTimes.get(key); + const startTime = starts?.shift(); // Get the earliest start time for this hook type + + // Cleanup empty arrays in map + if (starts && starts.length === 0) { + startTimes.delete(key); + } + + const now = Date.now(); + // Default to immediate removal if start time not found (defensive) + const elapsed = startTime ? now - startTime : WARNING_PROMPT_DURATION_MS; + const remaining = WARNING_PROMPT_DURATION_MS - elapsed; + + const removeHook = () => { + setActiveHooks((prev) => { + const index = prev.findIndex( + (h) => + h.name === payload.hookName && h.eventName === payload.eventName, + ); + if (index === -1) return prev; + const newHooks = [...prev]; + newHooks.splice(index, 1); + return newHooks; + }); + }; + + if (remaining > 0) { + const timeoutId = setTimeout(() => { + removeHook(); + activeTimeouts.delete(timeoutId); + }, remaining); + activeTimeouts.add(timeoutId); + } else { + removeHook(); + } + }; + + coreEvents.on(CoreEvent.HookStart, handleHookStart); + coreEvents.on(CoreEvent.HookEnd, handleHookEnd); + + return () => { + coreEvents.off(CoreEvent.HookStart, handleHookStart); + coreEvents.off(CoreEvent.HookEnd, handleHookEnd); + // Clear all pending timeouts + activeTimeouts.forEach(clearTimeout); + activeTimeouts.clear(); + }; + }, []); + + return activeHooks; +}; diff --git a/packages/cli/src/ui/types.ts b/packages/cli/src/ui/types.ts index c5947024f1..7535119a30 100644 --- a/packages/cli/src/ui/types.ts +++ b/packages/cli/src/ui/types.ts @@ -418,3 +418,10 @@ export interface ConfirmationRequest { export interface LoopDetectionConfirmationRequest { onComplete: (result: { userSelection: 'disable' | 'keep' }) => void; } + +export interface ActiveHook { + name: string; + eventName: string; + index?: number; + total?: number; +} diff --git a/packages/core/src/hooks/hookEventHandler.test.ts b/packages/core/src/hooks/hookEventHandler.test.ts index c6032298f6..2bffc805b6 100644 --- a/packages/core/src/hooks/hookEventHandler.test.ts +++ b/packages/core/src/hooks/hookEventHandler.test.ts @@ -31,6 +31,8 @@ const mockDebugLogger = vi.hoisted(() => ({ // Mock coreEvents const mockCoreEvents = vi.hoisted(() => ({ emitFeedback: vi.fn(), + emitHookStart: vi.fn(), + emitHookEnd: vi.fn(), })); vi.mock('../utils/debugLogger.js', () => ({ @@ -158,8 +160,31 @@ describe('HookEventHandler', () => { tool_name: 'EditTool', tool_input: { file: 'test.txt' }, }), + expect.any(Function), + expect.any(Function), ); + // Verify event emission via callbacks + const onHookStart = vi.mocked(mockHookRunner.executeHooksParallel).mock + .calls[0][3]; + const onHookEnd = vi.mocked(mockHookRunner.executeHooksParallel).mock + .calls[0][4]; + + if (onHookStart) onHookStart(mockPlan[0].hookConfig, 0); + expect(mockCoreEvents.emitHookStart).toHaveBeenCalledWith({ + hookName: './test.sh', + eventName: HookEventName.BeforeTool, + hookIndex: 1, + totalHooks: 1, + }); + + if (onHookEnd) onHookEnd(mockPlan[0].hookConfig, mockResults[0]); + expect(mockCoreEvents.emitHookEnd).toHaveBeenCalledWith({ + hookName: './test.sh', + eventName: HookEventName.BeforeTool, + success: true, + }); + expect(result).toBe(mockAggregated); }); @@ -294,6 +319,8 @@ describe('HookEventHandler', () => { tool_input: toolInput, tool_response: toolResponse, }), + expect.any(Function), + expect.any(Function), ); expect(result).toBe(mockAggregated); @@ -352,6 +379,8 @@ describe('HookEventHandler', () => { expect.objectContaining({ prompt, }), + expect.any(Function), + expect.any(Function), ); expect(result).toBe(mockAggregated); @@ -415,6 +444,8 @@ describe('HookEventHandler', () => { notification_type: 'ToolPermission', details: { type: 'ToolPermission', title: 'Test Permission' }, }), + expect.any(Function), + expect.any(Function), ); expect(result).toBe(mockAggregated); @@ -478,6 +509,8 @@ describe('HookEventHandler', () => { expect.objectContaining({ source: 'startup', }), + expect.any(Function), + expect.any(Function), ); expect(result).toBe(mockAggregated); @@ -548,6 +581,8 @@ describe('HookEventHandler', () => { ]), }), }), + expect.any(Function), + expect.any(Function), ); expect(result).toBe(mockAggregated); @@ -591,6 +626,8 @@ describe('HookEventHandler', () => { hook_event_name: 'BeforeTool', timestamp: expect.any(String), }), + expect.any(Function), + expect.any(Function), ); }); }); diff --git a/packages/core/src/hooks/hookEventHandler.ts b/packages/core/src/hooks/hookEventHandler.ts index 92268b7f51..e72aee913a 100644 --- a/packages/core/src/hooks/hookEventHandler.ts +++ b/packages/core/src/hooks/hookEventHandler.ts @@ -11,6 +11,7 @@ import type { HookRunner } from './hookRunner.js'; import type { HookAggregator, AggregatedHookResult } from './hookAggregator.js'; import { HookEventName } from './types.js'; import type { + HookConfig, HookInput, BeforeToolInput, AfterToolInput, @@ -507,17 +508,38 @@ export class HookEventHandler { }; } + const onHookStart = (config: HookConfig, index: number) => { + coreEvents.emitHookStart({ + hookName: this.getHookName(config), + eventName, + hookIndex: index + 1, + totalHooks: plan.hookConfigs.length, + }); + }; + + const onHookEnd = (config: HookConfig, result: HookExecutionResult) => { + coreEvents.emitHookEnd({ + hookName: this.getHookName(config), + eventName, + success: result.success, + }); + }; + // Execute hooks according to the plan's strategy const results = plan.sequential ? await this.hookRunner.executeHooksSequential( plan.hookConfigs, eventName, input, + onHookStart, + onHookEnd, ) : await this.hookRunner.executeHooksParallel( plan.hookConfigs, eventName, input, + onHookStart, + onHookEnd, ); // Aggregate results @@ -659,11 +681,18 @@ export class HookEventHandler { // Other common fields like decision/reason are handled by specific hook output classes } + /** + * Get hook name from config for display or telemetry + */ + private getHookName(config: HookConfig): string { + return config.name || config.command || 'unknown-command'; + } + /** * Get hook name from execution result for telemetry */ private getHookNameFromResult(result: HookExecutionResult): string { - return result.hookConfig.command || 'unknown-command'; + return this.getHookName(result.hookConfig); } /** diff --git a/packages/core/src/hooks/hookRunner.test.ts b/packages/core/src/hooks/hookRunner.test.ts index 090ae8e0d8..5bc671b088 100644 --- a/packages/core/src/hooks/hookRunner.test.ts +++ b/packages/core/src/hooks/hookRunner.test.ts @@ -434,6 +434,37 @@ describe('HookRunner', () => { expect(spawn).toHaveBeenCalledTimes(2); }); + it('should call onHookStart and onHookEnd callbacks', async () => { + const configs: HookConfig[] = [ + { name: 'hook1', type: HookType.Command, command: './hook1.sh' }, + ]; + + mockSpawn.mockProcessOn.mockImplementation( + (event: string, callback: (code: number) => void) => { + if (event === 'close') { + setImmediate(() => callback(0)); + } + }, + ); + + const onStart = vi.fn(); + const onEnd = vi.fn(); + + await hookRunner.executeHooksParallel( + configs, + HookEventName.BeforeTool, + mockInput, + onStart, + onEnd, + ); + + expect(onStart).toHaveBeenCalledWith(configs[0], 0); + expect(onEnd).toHaveBeenCalledWith( + configs[0], + expect.objectContaining({ success: true }), + ); + }); + it('should handle mixed success and failure', async () => { const configs: HookConfig[] = [ { type: HookType.Command, command: './hook1.sh' }, @@ -498,6 +529,37 @@ describe('HookRunner', () => { expect(executionOrder).toEqual(['./hook1.sh', './hook2.sh']); }); + it('should call onHookStart and onHookEnd callbacks sequentially', async () => { + const configs: HookConfig[] = [ + { name: 'hook1', type: HookType.Command, command: './hook1.sh' }, + { name: 'hook2', type: HookType.Command, command: './hook2.sh' }, + ]; + + mockSpawn.mockProcessOn.mockImplementation( + (event: string, callback: (code: number) => void) => { + if (event === 'close') { + setImmediate(() => callback(0)); + } + }, + ); + + const onStart = vi.fn(); + const onEnd = vi.fn(); + + await hookRunner.executeHooksSequential( + configs, + HookEventName.BeforeTool, + mockInput, + onStart, + onEnd, + ); + + expect(onStart).toHaveBeenCalledTimes(2); + expect(onEnd).toHaveBeenCalledTimes(2); + expect(onStart).toHaveBeenNthCalledWith(1, configs[0], 0); + expect(onStart).toHaveBeenNthCalledWith(2, configs[1], 1); + }); + it('should continue execution even if a hook fails', async () => { const configs: HookConfig[] = [ { type: HookType.Command, command: './hook1.sh' }, diff --git a/packages/core/src/hooks/hookRunner.ts b/packages/core/src/hooks/hookRunner.ts index 79aa4a1fd6..33d0404e6b 100644 --- a/packages/core/src/hooks/hookRunner.ts +++ b/packages/core/src/hooks/hookRunner.ts @@ -105,10 +105,15 @@ export class HookRunner { hookConfigs: HookConfig[], eventName: HookEventName, input: HookInput, + onHookStart?: (config: HookConfig, index: number) => void, + onHookEnd?: (config: HookConfig, result: HookExecutionResult) => void, ): Promise { - const promises = hookConfigs.map((config) => - this.executeHook(config, eventName, input), - ); + const promises = hookConfigs.map(async (config, index) => { + onHookStart?.(config, index); + const result = await this.executeHook(config, eventName, input); + onHookEnd?.(config, result); + return result; + }); return Promise.all(promises); } @@ -120,12 +125,17 @@ export class HookRunner { hookConfigs: HookConfig[], eventName: HookEventName, input: HookInput, + onHookStart?: (config: HookConfig, index: number) => void, + onHookEnd?: (config: HookConfig, result: HookExecutionResult) => void, ): Promise { const results: HookExecutionResult[] = []; let currentInput = input; - for (const config of hookConfigs) { + for (let i = 0; i < hookConfigs.length; i++) { + const config = hookConfigs[i]; + onHookStart?.(config, i); const result = await this.executeHook(config, eventName, currentInput); + onHookEnd?.(config, result); results.push(result); // If the hook succeeded and has output, use it to modify the input for the next hook diff --git a/packages/core/src/utils/events.test.ts b/packages/core/src/utils/events.test.ts index 497bec398f..5b84af0628 100644 --- a/packages/core/src/utils/events.test.ts +++ b/packages/core/src/utils/events.test.ts @@ -274,4 +274,35 @@ describe('CoreEventEmitter', () => { expect(listener).toHaveBeenCalledWith({ model: newModel }); }); }); + + describe('Hook Events', () => { + it('should emit HookStart event with correct payload using helper', () => { + const listener = vi.fn(); + events.on(CoreEvent.HookStart, listener); + + const payload = { + hookName: 'test-hook', + eventName: 'before-agent', + hookIndex: 1, + totalHooks: 1, + }; + events.emitHookStart(payload); + + expect(listener).toHaveBeenCalledWith(payload); + }); + + it('should emit HookEnd event with correct payload using helper', () => { + const listener = vi.fn(); + events.on(CoreEvent.HookEnd, listener); + + const payload = { + hookName: 'test-hook', + eventName: 'before-agent', + success: true, + }; + events.emitHookEnd(payload); + + expect(listener).toHaveBeenCalledWith(payload); + }); + }); }); diff --git a/packages/core/src/utils/events.ts b/packages/core/src/utils/events.ts index aebc1901a2..84058f9d99 100644 --- a/packages/core/src/utils/events.ts +++ b/packages/core/src/utils/events.ts @@ -67,6 +67,36 @@ export interface MemoryChangedPayload { fileCount: number; } +/** + * Base payload for hook-related events. + */ +export interface HookPayload { + hookName: string; + eventName: string; +} + +/** + * Payload for the 'hook-start' event. + */ +export interface HookStartPayload extends HookPayload { + /** + * The 1-based index of the current hook in the execution sequence. + * Used for progress indication (e.g. "Hook 1/3"). + */ + hookIndex?: number; + /** + * The total number of hooks in the current execution sequence. + */ + totalHooks?: number; +} + +/** + * Payload for the 'hook-end' event. + */ +export interface HookEndPayload extends HookPayload { + success: boolean; +} + export enum CoreEvent { UserFeedback = 'user-feedback', ModelChanged = 'model-changed', @@ -75,6 +105,8 @@ export enum CoreEvent { MemoryChanged = 'memory-changed', ExternalEditorClosed = 'external-editor-closed', SettingsChanged = 'settings-changed', + HookStart = 'hook-start', + HookEnd = 'hook-end', } export interface CoreEvents { @@ -85,6 +117,8 @@ export interface CoreEvents { [CoreEvent.MemoryChanged]: [MemoryChangedPayload]; [CoreEvent.ExternalEditorClosed]: never[]; [CoreEvent.SettingsChanged]: never[]; + [CoreEvent.HookStart]: [HookStartPayload]; + [CoreEvent.HookEnd]: [HookEndPayload]; } type EventBacklogItem = { @@ -172,6 +206,20 @@ export class CoreEventEmitter extends EventEmitter { this.emit(CoreEvent.SettingsChanged); } + /** + * Notifies subscribers that a hook execution has started. + */ + emitHookStart(payload: HookStartPayload): void { + this.emit(CoreEvent.HookStart, payload); + } + + /** + * Notifies subscribers that a hook execution has ended. + */ + emitHookEnd(payload: HookEndPayload): void { + this.emit(CoreEvent.HookEnd, payload); + } + /** * Flushes buffered messages. Call this immediately after primary UI listener * subscribes. diff --git a/schemas/settings.schema.json b/schemas/settings.schema.json index 4900fa25d6..7c6f1cf3c7 100644 --- a/schemas/settings.schema.json +++ b/schemas/settings.schema.json @@ -1512,6 +1512,13 @@ "type": "string" } }, + "notifications": { + "title": "Hook Notifications", + "description": "Show visual indicators when hooks are executing.", + "markdownDescription": "Show visual indicators when hooks are executing.\n\n- Category: `Advanced`\n- Requires restart: `no`\n- Default: `true`", + "default": true, + "type": "boolean" + }, "BeforeTool": { "title": "Before Tool Hooks", "description": "Hooks that execute before tool execution. Can intercept, validate, or modify tool calls.",