Skip to content
83 changes: 82 additions & 1 deletion packages/cli/src/ui/commands/extensionsCommand.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,15 @@ import { createMockCommandContext } from '../../test-utils/mockCommandContext.js
import { MessageType } from '../types.js';
import { extensionsCommand } from './extensionsCommand.js';
import { type CommandContext } from './types.js';
import { describe, it, expect, vi, beforeEach } from 'vitest';
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
import { type ExtensionUpdateAction } from '../state/extensions.js';

// Mock the 'open' library similar to docsCommand.test.ts
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove this comment

import open from 'open';
vi.mock('open', () => ({
default: vi.fn(),
}));

vi.mock('../../config/extensions/update.js', () => ({
updateExtension: vi.fn(),
checkForAllExtensionUpdates: vi.fn(),
Expand All @@ -26,6 +32,8 @@ describe('extensionsCommand', () => {
beforeEach(() => {
vi.resetAllMocks();
mockGetExtensions.mockReturnValue([]);
// Reset the `open` mock as done in docsCommand.test.ts
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

vi.mocked(open).mockClear();
mockContext = createMockCommandContext({
services: {
config: {
Expand All @@ -39,6 +47,11 @@ describe('extensionsCommand', () => {
});
});

afterEach(() => {
// Restore any stubbed environment variables, similar to docsCommand.test.ts
vi.unstubAllEnvs();
});

describe('list', () => {
it('should add an EXTENSIONS_LIST item to the UI', async () => {
if (!extensionsCommand.action) throw new Error('Action not defined');
Expand Down Expand Up @@ -302,4 +315,72 @@ describe('extensionsCommand', () => {
});
});
});

describe('explore', () => {
const exploreAction = extensionsCommand.subCommands?.find(
(cmd) => cmd.name === 'explore',
)?.action;

if (!exploreAction) {
throw new Error('Explore action not found');
}

it("should add an info message and call 'open' in a non-sandbox environment", async () => {
// Ensure no special environment variables that would affect behavior
vi.stubEnv('NODE_ENV', '');
vi.stubEnv('SANDBOX', '');

await exploreAction(mockContext, '');

const extensionsUrl = 'https://geminicli.com/extensions/';
expect(mockContext.ui.addItem).toHaveBeenCalledWith(
{
type: MessageType.INFO,
text: `Opening extensions page in your browser: ${extensionsUrl}`,
},
expect.any(Number),
);

expect(open).toHaveBeenCalledWith(extensionsUrl);
});

it('should only add an info message in a sandbox environment', async () => {
// Simulate a sandbox environment
vi.stubEnv('NODE_ENV', '');
vi.stubEnv('SANDBOX', 'gemini-sandbox');
const extensionsUrl = 'https://geminicli.com/extensions/';

await exploreAction(mockContext, '');

expect(mockContext.ui.addItem).toHaveBeenCalledWith(
{
type: MessageType.INFO,
text: `Please open the following URL in your browser to explore extensions:\\n${extensionsUrl}`,
},
expect.any(Number),
);

// Ensure 'open' was not called in the sandbox
expect(open).not.toHaveBeenCalled();
});

it('should add an info message and not call open in NODE_ENV test environment', async () => {
vi.stubEnv('NODE_ENV', 'test');
vi.stubEnv('SANDBOX', '');
const extensionsUrl = 'https://geminicli.com/extensions/';

await exploreAction(mockContext, '');

expect(mockContext.ui.addItem).toHaveBeenCalledWith(
{
type: MessageType.INFO,
text: `Would open extensions page in your browser: ${extensionsUrl} (skipped in test environment)`,
},
expect.any(Number),
);

// Ensure 'open' was not called in test environment
expect(open).not.toHaveBeenCalled();
});
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

While the existing tests cover the success paths well, a key scenario is missing. It's important to also test the failure case where open() might reject (e.g., if a browser cannot be opened). Adding a test for this scenario would ensure your new error handling logic works as expected and prevents unhandled promise rejections.

});
50 changes: 49 additions & 1 deletion packages/cli/src/ui/commands/extensionsCommand.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import {
type SlashCommand,
CommandKind,
} from './types.js';
import open from 'open';
import process from 'node:process';

async function listAction(context: CommandContext) {
const historyItem: HistoryItemExtensionsList = {
Expand Down Expand Up @@ -112,6 +114,41 @@ function updateAction(context: CommandContext, args: string): Promise<void> {
return updateComplete.then((_) => {});
}

async function exploreAction(context: CommandContext) {
const extensionsUrl = 'https://geminicli.com/extensions/';

// Only check for NODE_ENV for explicit test mode, not for unit test framework
if (process.env['NODE_ENV'] === 'test') {
context.ui.addItem(
{
type: MessageType.INFO,
text: `Would open extensions page in your browser: ${extensionsUrl} (skipped in test environment)`,
},
Date.now(),
);
} else if (
process.env['SANDBOX'] &&
process.env['SANDBOX'] !== 'sandbox-exec'
) {
context.ui.addItem(
{
type: MessageType.INFO,
text: `Please open the following URL in your browser to explore extensions:\\n${extensionsUrl}`,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"View available extensions at ${extensionsUrl}"

},
Date.now(),
);
} else {
context.ui.addItem(
{
type: MessageType.INFO,
text: `Opening extensions page in your browser: ${extensionsUrl}`,
},
Date.now(),
);
await open(extensionsUrl);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The call to open() can fail for various reasons (e.g., no default browser configured, permissions issues). An unhandled promise rejection here could lead to an uncaught exception, potentially providing a poor user experience. It's better to wrap this call in a try...catch block to gracefully handle any errors and inform the user.

    try {
      await open(extensionsUrl);
    } catch (error) {
      context.ui.addItem(
        {
          type: MessageType.ERROR,
          text: `Failed to open browser. Please open this URL manually: ${extensionsUrl}`,
        },
        Date.now()
      );
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks !

}
}

const listExtensionsCommand: SlashCommand = {
name: 'list',
description: 'List active extensions',
Expand Down Expand Up @@ -141,11 +178,22 @@ const updateExtensionsCommand: SlashCommand = {
},
};

const exploreExtensionsCommand: SlashCommand = {
name: 'explore',
description: 'Open extensions page in your browser',
kind: CommandKind.BUILT_IN,
action: exploreAction,
};

export const extensionsCommand: SlashCommand = {
name: 'extensions',
description: 'Manage extensions',
kind: CommandKind.BUILT_IN,
subCommands: [listExtensionsCommand, updateExtensionsCommand],
subCommands: [
listExtensionsCommand,
updateExtensionsCommand,
exploreExtensionsCommand,
],
action: (context, args) =>
// Default to list if no subcommand is provided
listExtensionsCommand.action!(context, args),
Expand Down