Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 71 additions & 0 deletions packages/cli/src/services/CommandService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -350,4 +350,75 @@ describe('CommandService', () => {
expect(deployExtension).toBeDefined();
expect(deployExtension?.description).toBe('[gcp] Deploy to Google Cloud');
});

it('should report conflicts via getConflicts', async () => {
const builtinCommand = createMockCommand('deploy', CommandKind.BUILT_IN);
const extensionCommand = {
...createMockCommand('deploy', CommandKind.FILE),
extensionName: 'firebase',
};

const mockLoader = new MockCommandLoader([
builtinCommand,
extensionCommand,
]);

const service = await CommandService.create(
[mockLoader],
new AbortController().signal,
);

const conflicts = service.getConflicts();
expect(conflicts).toHaveLength(1);

expect(conflicts[0]).toMatchObject({
name: 'deploy',
renamedTo: 'firebase.deploy',
});
expect(conflicts[0].winner).toEqual(builtinCommand);
expect(conflicts[0].loser).toMatchObject({
name: 'deploy',
extensionName: 'firebase',
});
});

it('should report extension vs extension conflicts correctly', async () => {
// Both extensions try to register 'deploy'
const extension1Command = {
...createMockCommand('deploy', CommandKind.FILE),
extensionName: 'firebase',
};
const extension2Command = {
...createMockCommand('deploy', CommandKind.FILE),
extensionName: 'aws',
};

const mockLoader = new MockCommandLoader([
extension1Command,
extension2Command,
]);

const service = await CommandService.create(
[mockLoader],
new AbortController().signal,
);

const conflicts = service.getConflicts();
expect(conflicts).toHaveLength(1);

expect(conflicts[0]).toMatchObject({
name: 'deploy',
renamedTo: 'aws.deploy', // ext2 is 'aws' and it lost because it was second in the list
});

expect(conflicts[0].winner).toMatchObject({
name: 'deploy',
extensionName: 'firebase',
});

expect(conflicts[0].loser).toMatchObject({
name: 'deploy',
extensionName: 'aws',
});
});
});
35 changes: 33 additions & 2 deletions packages/cli/src/services/CommandService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,13 @@ import { debugLogger } from '@google/gemini-cli-core';
import type { SlashCommand } from '../ui/commands/types.js';
import type { ICommandLoader } from './types.js';

export interface CommandConflict {
name: string;
winner: SlashCommand;
Comment thread
chrstnb marked this conversation as resolved.
Resolved
loser: SlashCommand;
renamedTo: string;
}

/**
* Orchestrates the discovery and loading of all slash commands for the CLI.
*
Expand All @@ -23,8 +30,12 @@ export class CommandService {
/**
* Private constructor to enforce the use of the async factory.
* @param commands A readonly array of the fully loaded and de-duplicated commands.
* @param conflicts A readonly array of conflicts that occurred during loading.
*/
private constructor(private readonly commands: readonly SlashCommand[]) {}
private constructor(
private readonly commands: readonly SlashCommand[],
private readonly conflicts: readonly CommandConflict[],
) {}

/**
* Asynchronously creates and initializes a new CommandService instance.
Expand Down Expand Up @@ -63,11 +74,14 @@ export class CommandService {
}

const commandMap = new Map<string, SlashCommand>();
const conflicts: CommandConflict[] = [];

for (const cmd of allCommands) {
let finalName = cmd.name;

// Extension commands get renamed if they conflict with existing commands
if (cmd.extensionName && commandMap.has(cmd.name)) {
const winner = commandMap.get(cmd.name)!;
let renamedName = `${cmd.extensionName}.${cmd.name}`;
let suffix = 1;

Expand All @@ -78,6 +92,13 @@ export class CommandService {
}

finalName = renamedName;

conflicts.push({
name: cmd.name,
winner,
loser: cmd,
renamedTo: finalName,
});
}

commandMap.set(finalName, {
Expand All @@ -87,7 +108,8 @@ export class CommandService {
}

const finalCommands = Object.freeze(Array.from(commandMap.values()));
return new CommandService(finalCommands);
const finalConflicts = Object.freeze(conflicts);
return new CommandService(finalCommands, finalConflicts);
}

/**
Expand All @@ -101,4 +123,13 @@ export class CommandService {
getCommands(): readonly SlashCommand[] {
return this.commands;
}

/**
* Retrieves the list of conflicts that occurred during command loading.
*
* @returns A readonly array of command conflicts.
*/
getConflicts(): readonly CommandConflict[] {
return this.conflicts;
}
}
115 changes: 115 additions & 0 deletions packages/cli/src/ui/hooks/slashCommandProcessor.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1050,4 +1050,119 @@ describe('useSlashCommandProcessor', () => {
expect(result.current.slashCommands).toEqual([newCommand]),
);
});

describe('Conflict Notifications', () => {
it('should display a warning when a command conflict occurs', async () => {
const builtinCommand = createTestCommand({ name: 'deploy' });
const extensionCommand = createTestCommand(
{
name: 'deploy',
extensionName: 'firebase',
},
CommandKind.FILE,
);

const result = await setupProcessorHook({
builtinCommands: [builtinCommand],
fileCommands: [extensionCommand],
});

await waitFor(() => expect(result.current.slashCommands).toHaveLength(2));

expect(mockAddItem).toHaveBeenCalledWith(
expect.objectContaining({
type: MessageType.INFO,
text: expect.stringContaining('Command conflicts detected'),
}),
expect.any(Number),
);

expect(mockAddItem).toHaveBeenCalledWith(
expect.objectContaining({
type: MessageType.INFO,
text: expect.stringContaining(
"- Command '/deploy' from extension 'firebase' was renamed",
),
}),
expect.any(Number),
);
});

it('should deduplicate conflict warnings across re-renders', async () => {
const builtinCommand = createTestCommand({ name: 'deploy' });
const extensionCommand = createTestCommand(
{
name: 'deploy',
extensionName: 'firebase',
},
CommandKind.FILE,
);

const result = await setupProcessorHook({
builtinCommands: [builtinCommand],
fileCommands: [extensionCommand],
});

await waitFor(() => expect(result.current.slashCommands).toHaveLength(2));

// First notification
expect(mockAddItem).toHaveBeenCalledWith(
expect.objectContaining({
type: MessageType.INFO,
text: expect.stringContaining('Command conflicts detected'),
}),
expect.any(Number),
);

mockAddItem.mockClear();

// Trigger a reload or re-render
await act(async () => {
result.current.commandContext.ui.reloadCommands();
});

// Wait a bit for effect to run
await new Promise((resolve) => setTimeout(resolve, 100));

// Should NOT have notified again
expect(mockAddItem).not.toHaveBeenCalledWith(
expect.objectContaining({
type: MessageType.INFO,
text: expect.stringContaining('Command conflicts detected'),
}),
expect.any(Number),
);
});

it('should correctly identify the winner extension in the message', async () => {
const ext1Command = createTestCommand(
{
name: 'deploy',
extensionName: 'firebase',
},
CommandKind.FILE,
);
const ext2Command = createTestCommand(
{
name: 'deploy',
extensionName: 'aws',
},
CommandKind.FILE,
);

const result = await setupProcessorHook({
fileCommands: [ext1Command, ext2Command],
});

await waitFor(() => expect(result.current.slashCommands).toHaveLength(2));

expect(mockAddItem).toHaveBeenCalledWith(
expect.objectContaining({
type: MessageType.INFO,
text: expect.stringContaining("conflicts with extension 'firebase'"),
}),
expect.any(Number),
);
});
});
});
33 changes: 32 additions & 1 deletion packages/cli/src/ui/hooks/slashCommandProcessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
useMemo,
useEffect,
useState,
useRef,
createElement,
} from 'react';
import { type PartListUnion } from '@google/genai';
Expand Down Expand Up @@ -108,6 +109,7 @@ export const useSlashCommandProcessor = (
undefined,
);
const [reloadTrigger, setReloadTrigger] = useState(0);
const notifiedConflictsRef = useRef(new Set<string>());

const reloadCommands = useCallback(() => {
setReloadTrigger((v) => v + 1);
Expand Down Expand Up @@ -324,12 +326,41 @@ export const useSlashCommandProcessor = (
controller.signal,
);
setCommands(commandService.getCommands());

const conflicts = commandService.getConflicts();
const newConflicts = conflicts.filter((c) => {
const key = `${c.name}:${c.loser.extensionName}`;
if (notifiedConflictsRef.current.has(key)) {
return false;
}
notifiedConflictsRef.current.add(key);
return true;
});

if (newConflicts.length > 0) {
const conflictMessages = newConflicts
.map((c) => {
const winnerSource = c.winner.extensionName
? `extension '${c.winner.extensionName}'`
: 'an existing command';
return `- Command '/${c.name}' from extension '${c.loser.extensionName}' was renamed to '/${c.renamedTo}' because it conflicts with ${winnerSource}.`;
})
.join('\n');

addItem(
Comment thread
chrstnb marked this conversation as resolved.
Resolved
{
type: MessageType.INFO,
text: `Command conflicts detected:\n${conflictMessages}`,
},
Date.now(),
);
}
})();

return () => {
controller.abort();
};
}, [config, reloadTrigger, isConfigInitialized]);
}, [config, reloadTrigger, isConfigInitialized, addItem]);
Comment thread
chrstnb marked this conversation as resolved.
Resolved

const handleSlashCommand = useCallback(
async (
Expand Down
Loading