mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-04-21 18:44:30 -07:00
feat(ui): add visual indicators for hook execution (#15408)
This commit is contained in:
@@ -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),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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' },
|
||||
|
||||
@@ -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<HookExecutionResult[]> {
|
||||
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<HookExecutionResult[]> {
|
||||
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
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<CoreEvents> {
|
||||
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.
|
||||
|
||||
Reference in New Issue
Block a user