MediaMetz/gemini-cli
1
0
Fork 0
mirror of https://github.com/google-gemini/gemini-cli.git synced 2026-03-29 07:21:27 -07:00
Code Issues Packages Projects Releases Wiki Activity

address comments by bot

Browse Source
This commit is contained in:
A.K.M. Adib
2026-01-27 12:36:27 -05:00
parent 322b866a5a
commit f5bf268117
12 changed files with 413 additions and 109 deletions
Download Patch File Download Diff File Expand all files Collapse all files

13
packages/cli/src/ui/commands/planCommand.ts
View File

@@ -6,6 +6,7 @@
import * as fs from 'node:fs';
import * as path from 'node:path';
import { isDirectorySecure } from '@google/gemini-cli-core';
import type { SlashCommand } from './types.js';
import { CommandKind } from './types.js';
import { MessageType } from '../types.js';
@@ -26,6 +27,18 @@ export const planCommand: SlashCommand = {
const plansDir = config.storage.getProjectTempPlansDir();
try {
const securityCheck = await isDirectorySecure(plansDir);
if (!securityCheck.secure) {
context.ui.addItem(
{
type: MessageType.ERROR,
text: `Security check failed for plans directory: ${securityCheck.reason}`,
},
Date.now(),
);
return;
}
if (fs.existsSync(plansDir)) {
const files = await fs.promises.readdir(plansDir);
const mdFiles = files.filter((f) => f.endsWith('.md'));

92
packages/cli/src/ui/components/messages/Todo.test.tsx
View File

@@ -4,15 +4,13 @@
* SPDX-License-Identifier: Apache-2.0
*/
import { render } from '../../../test-utils/render.js';
import { renderWithProviders as render } from '../../../test-utils/render.js';
import { describe, it, expect } from 'vitest';
import { Box } from 'ink';
import { TodoTray } from './Todo.js';
import type { Todo } from '@google/gemini-cli-core';
import type { UIState } from '../../contexts/UIStateContext.js';
import { UIStateContext } from '../../contexts/UIStateContext.js';
import type { HistoryItem } from '../../types.js';
import { ToolCallStatus } from '../../types.js';
import type { UIState } from '../../contexts/UIStateContext.js';
const createTodoHistoryItem = (todos: Todo[]): HistoryItem =>
({
@@ -34,21 +32,18 @@ describe.each([true, false])(
'<TodoTray /> (showFullTodos: %s)',
(showFullTodos: boolean) => {
const renderWithUiState = (uiState: Partial<UIState>) =>
render(
<UIStateContext.Provider value={uiState as UIState}>
<TodoTray />
</UIStateContext.Provider>,
);
render(<TodoTray />, {
uiState: { ...uiState, showFullTodos },
});
it('renders null when no todos are in the history', () => {
const { lastFrame } = renderWithUiState({ history: [], showFullTodos });
const { lastFrame } = renderWithUiState({ history: [] });
expect(lastFrame()).toMatchSnapshot();
});
it('renders null when todo list is empty', () => {
const { lastFrame } = renderWithUiState({
history: [createTodoHistoryItem([])],
showFullTodos,
});
expect(lastFrame()).toMatchSnapshot();
});
@@ -62,7 +57,6 @@ describe.each([true, false])(
{ description: 'Completed Task', status: 'completed' },
]),
],
showFullTodos,
});
expect(lastFrame()).toMatchSnapshot();
});
@@ -77,39 +71,31 @@ describe.each([true, false])(
{ description: 'Completed Task', status: 'completed' },
]),
],
showFullTodos,
});
expect(lastFrame()).toMatchSnapshot();
});
it('renders a todo list with long descriptions that wrap when full view is on', () => {
const { lastFrame } = render(
<Box width="50">
<UIStateContext.Provider
value={
const { lastFrame } = render(<TodoTray />, {
width: 50,
uiState: {
history: [
createTodoHistoryItem([
{
history: [
createTodoHistoryItem([
{
description:
'This is a very long description for a pending task that should wrap around multiple lines when the terminal width is constrained.',
status: 'in_progress',
},
{
description:
'Another completed task with an equally verbose description to test wrapping behavior.',
status: 'completed',
},
]),
],
showFullTodos,
} as UIState
}
>
<TodoTray />
</UIStateContext.Provider>
</Box>,
);
description:
'This is a very long description for a pending task that should wrap around multiple lines when the terminal width is constrained.',
status: 'in_progress',
},
{
description:
'Another completed task with an equally verbose description to test wrapping behavior.',
status: 'completed',
},
]),
],
showFullTodos,
},
});
expect(lastFrame()).toMatchSnapshot();
});
@@ -125,7 +111,6 @@ describe.each([true, false])(
{ description: 'Newer Task 2', status: 'in_progress' },
]),
],
showFullTodos,
});
expect(lastFrame()).toMatchSnapshot();
});
@@ -138,9 +123,34 @@ describe.each([true, false])(
{ description: 'Task 2', status: 'cancelled' },
]),
],
showFullTodos,
});
expect(lastFrame()).toMatchSnapshot();
});
it('renders planTodos instead of history todos when available', () => {
const { lastFrame } = renderWithUiState({
history: [
createTodoHistoryItem([
{ description: 'History Task', status: 'pending' },
]),
],
planTodos: [
{ description: 'Plan Task 1', status: 'in_progress' },
{ description: 'Plan Task 2', status: 'pending' },
],
planFileName: 'implementation.md',
});
expect(lastFrame()).toMatchSnapshot();
});
it('renders planTodos with the correct filename in the header', () => {
const { lastFrame } = renderWithUiState({
planTodos: [{ description: 'Only Task', status: 'pending' }],
planFileName: 'my-feature.md',
showFullTodos: true,
});
expect(lastFrame()).toContain('Plan: my-feature.md');
expect(lastFrame()).toMatchSnapshot();
});
},
);

3
packages/cli/src/ui/components/messages/Todo.tsx
View File

@@ -6,6 +6,7 @@
import type React from 'react';
import { Box, Text } from 'ink';
import stripAnsi from 'strip-ansi';
import {
type Todo,
type TodoList,
@@ -37,7 +38,7 @@ const TodoTitleDisplay: React.FC<{
return (
<Box flexDirection="row" columnGap={2} height={1}>
<Text color={theme.text.primary} bold aria-label="Todo list">
{fileName ? `Plan: ${fileName}` : 'Todo'}
{fileName ? `Plan: ${stripAnsi(fileName)}` : 'Todo'}
</Text>
<Text color={theme.text.secondary}>{score} (ctrl+t to toggle)</Text>
</Box>

39
packages/cli/src/ui/components/messages/__snapshots__/Todo.test.tsx.snap
View File

@@ -11,18 +11,28 @@ exports[`<TodoTray /> (showFullTodos: false) > renders null when no todos are in
exports[`<TodoTray /> (showFullTodos: false) > renders null when todo list is empty 1`] = `""`;
exports[`<TodoTray /> (showFullTodos: false) > renders planTodos instead of history todos when available 1`] = `
"────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Plan: implementation.md 0/2 completed (ctrl+t to toggle) » Plan Task 1"
`;
exports[`<TodoTray /> (showFullTodos: false) > renders planTodos with the correct filename in the header 1`] = `
"────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Plan: my-feature.md 0/1 completed (ctrl+t to toggle)"
`;
exports[`<TodoTray /> (showFullTodos: false) > renders the most recent todo list when multiple write_todos calls are in history 1`] = `
"────────────────────────────────────────────────────────────────────────────────────────────────────
"────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Todo 0/2 completed (ctrl+t to toggle) » Newer Task 2"
`;
exports[`<TodoTray /> (showFullTodos: false) > renders when todos exist and one is in progress 1`] = `
"────────────────────────────────────────────────────────────────────────────────────────────────────
"────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Todo 1/3 completed (ctrl+t to toggle) » Task 2"
`;
exports[`<TodoTray /> (showFullTodos: false) > renders when todos exist but none are in progress 1`] = `
"────────────────────────────────────────────────────────────────────────────────────────────────────
"────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Todo 1/2 completed (ctrl+t to toggle)"
`;
@@ -38,7 +48,7 @@ exports[`<TodoTray /> (showFullTodos: true) > renders a todo list with long desc
`;
exports[`<TodoTray /> (showFullTodos: true) > renders full list when all todos are inactive 1`] = `
"────────────────────────────────────────────────────────────────────────────────────────────────────
"────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Todo 1/1 completed (ctrl+t to toggle)
✓ Task 1
@@ -49,8 +59,23 @@ exports[`<TodoTray /> (showFullTodos: true) > renders null when no todos are in
exports[`<TodoTray /> (showFullTodos: true) > renders null when todo list is empty 1`] = `""`;
exports[`<TodoTray /> (showFullTodos: true) > renders planTodos instead of history todos when available 1`] = `
"────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Plan: implementation.md 0/2 completed (ctrl+t to toggle)
» Plan Task 1
☐ Plan Task 2"
`;
exports[`<TodoTray /> (showFullTodos: true) > renders planTodos with the correct filename in the header 1`] = `
"────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Plan: my-feature.md 0/1 completed (ctrl+t to toggle)
☐ Only Task"
`;
exports[`<TodoTray /> (showFullTodos: true) > renders the most recent todo list when multiple write_todos calls are in history 1`] = `
"────────────────────────────────────────────────────────────────────────────────────────────────────
"────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Todo 0/2 completed (ctrl+t to toggle)
☐ Newer Task 1
@@ -58,7 +83,7 @@ exports[`<TodoTray /> (showFullTodos: true) > renders the most recent todo list
`;
exports[`<TodoTray /> (showFullTodos: true) > renders when todos exist and one is in progress 1`] = `
"────────────────────────────────────────────────────────────────────────────────────────────────────
"────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Todo 1/3 completed (ctrl+t to toggle)
☐ Pending Task
@@ -68,7 +93,7 @@ exports[`<TodoTray /> (showFullTodos: true) > renders when todos exist and one i
`;
exports[`<TodoTray /> (showFullTodos: true) > renders when todos exist but none are in progress 1`] = `
"────────────────────────────────────────────────────────────────────────────────────────────────────
"────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Todo 1/2 completed (ctrl+t to toggle)
☐ Pending Task

176
packages/cli/src/ui/hooks/usePlanMonitoring.test.ts Normal file
View File

@@ -0,0 +1,176 @@
/**
* @license
* Copyright 2025 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
import { usePlanMonitoring } from './usePlanMonitoring.js';
import { renderHook } from '../../test-utils/render.js';
import * as fs from 'node:fs';
import * as path from 'node:path';
import * as os from 'node:os';
import { makeFakeConfig, type Config } from '@google/gemini-cli-core';
import { waitFor } from '../../test-utils/async.js';
import { act } from 'react';
const { mockFsPromises } = vi.hoisted(() => ({
mockFsPromises: {
readdir: vi.fn(),
lstat: vi.fn(),
readFile: vi.fn(),
stat: vi.fn(),
},
}));
vi.mock('node:fs', () => ({
existsSync: vi.fn(),
lstatSync: vi.fn(),
statSync: vi.fn(),
realpathSync: vi.fn(),
promises: mockFsPromises,
default: {
existsSync: vi.fn(),
lstatSync: vi.fn(),
statSync: vi.fn(),
realpathSync: vi.fn(),
promises: mockFsPromises,
},
}));
vi.mock('node:fs/promises', () => mockFsPromises);
describe('usePlanMonitoring', () => {
let mockConfig: Config;
const plansDir = path.join(os.tmpdir(), 'test-plans');
beforeEach(() => {
vi.useFakeTimers({ shouldAdvanceTime: true });
vi.mocked(fs.existsSync).mockImplementation((p) => {
const pathStr = p.toString();
if (pathStr === plansDir || pathStr === os.tmpdir()) return true;
return false;
});
vi.mocked(fs.lstatSync).mockImplementation((p) => {
const pathStr = p.toString();
if (pathStr === plansDir || pathStr === os.tmpdir()) {
return { isDirectory: () => true } as fs.Stats;
}
throw new Error('ENOENT');
});
vi.mocked(fs.statSync).mockImplementation((p) => {
const pathStr = p.toString();
if (pathStr === plansDir || pathStr === os.tmpdir()) {
return { isDirectory: () => true } as fs.Stats;
}
throw new Error('ENOENT');
});
vi.mocked(fs.realpathSync).mockImplementation((p) => p.toString());
mockConfig = makeFakeConfig({
targetDir: os.tmpdir(),
});
vi.spyOn(mockConfig.storage, 'getProjectTempPlansDir').mockReturnValue(
plansDir,
);
mockFsPromises.readdir.mockResolvedValue([]);
});
afterEach(() => {
vi.restoreAllMocks();
vi.useRealTimers();
});
it('initially has null todos and planFileName', async () => {
mockFsPromises.readdir.mockResolvedValue([]);
const { result } = renderHook(() => usePlanMonitoring(mockConfig));
expect(result.current.planTodos).toBeNull();
expect(result.current.planFileName).toBeNull();
});
it('reads the most recently modified markdown file', async () => {
const file1 = 'old.md';
const file2 = 'new.md';
mockFsPromises.readdir.mockResolvedValue([file1, file2]);
mockFsPromises.lstat.mockImplementation(async (p) => {
const pathStr = p.toString();
if (pathStr.endsWith(file1))
return { isFile: () => true, mtimeMs: 100 } as fs.Stats;
if (pathStr.endsWith(file2))
return { isFile: () => true, mtimeMs: 200 } as fs.Stats;
return { isFile: () => false } as fs.Stats;
});
mockFsPromises.readFile.mockResolvedValue('- [ ] Task from new.md');
const { result } = renderHook(() => usePlanMonitoring(mockConfig));
await waitFor(() => {
expect(result.current.planFileName).toBe(file2);
expect(result.current.planTodos).toEqual([
{ description: 'Task from new.md', status: 'pending' },
]);
});
});
it('updates todos when the file content changes', async () => {
mockFsPromises.readdir.mockResolvedValue(['plan.md']);
mockFsPromises.lstat.mockResolvedValue({
isFile: () => true,
mtimeMs: 100,
} as fs.Stats);
mockFsPromises.readFile.mockResolvedValueOnce('- [ ] Initial Task');
const { result } = renderHook(() => usePlanMonitoring(mockConfig));
await waitFor(() => {
expect(result.current.planTodos?.[0].description).toBe('Initial Task');
});
// Change mtime and content
mockFsPromises.lstat.mockResolvedValue({
isFile: () => true,
mtimeMs: 200,
} as fs.Stats);
mockFsPromises.readFile.mockResolvedValueOnce('- [x] Completed Task');
// Advance timers to trigger interval
await act(async () => {
await vi.advanceTimersByTimeAsync(2000);
});
await waitFor(() => {
expect(result.current.planTodos?.[0].description).toBe('Completed Task');
expect(result.current.planTodos?.[0].status).toBe('completed');
});
});
it('clears todos when no markdown files are found', async () => {
mockFsPromises.readdir.mockResolvedValueOnce(['plan.md']);
mockFsPromises.lstat.mockResolvedValue({
isFile: () => true,
mtimeMs: 100,
} as fs.Stats);
mockFsPromises.readFile.mockResolvedValue('- [ ] Task');
const { result } = renderHook(() => usePlanMonitoring(mockConfig));
await waitFor(() => {
expect(result.current.planTodos).not.toBeNull();
});
mockFsPromises.readdir.mockResolvedValueOnce([]);
await act(async () => {
await vi.advanceTimersByTimeAsync(2000);
});
await waitFor(() => {
expect(result.current.planTodos).toBeNull();
expect(result.current.planFileName).toBeNull();
});
});
});

27
packages/cli/src/ui/hooks/usePlanMonitoring.ts
View File

@@ -12,11 +12,13 @@ import {
type Todo,
type Config,
debugLogger,
isDirectorySecure,
} from '@google/gemini-cli-core';
export function usePlanMonitoring(config: Config) {
const [planTodos, setPlanTodos] = useState<Todo[] | null>(null);
const [planFileName, setPlanFileName] = useState<string | null>(null);
const planFileNameRef = useRef<string | null>(null);
const lastModifiedRef = useRef<number>(0);
useEffect(() => {
@@ -24,6 +26,15 @@ export function usePlanMonitoring(config: Config) {
const updatePlan = async () => {
try {
const securityCheck = await isDirectorySecure(plansDir);
if (!securityCheck.secure) {
debugLogger.warn(
'Security check failed for plans directory',
securityCheck.reason,
);
return;
}
if (!fs.existsSync(plansDir)) {
return;
}
@@ -32,8 +43,11 @@ export function usePlanMonitoring(config: Config) {
const mdFiles = files.filter((f) => f.endsWith('.md'));
if (mdFiles.length === 0) {
setPlanTodos(null);
setPlanFileName(null);
if (planFileNameRef.current !== null) {
setPlanTodos(null);
setPlanFileName(null);
planFileNameRef.current = null;
}
return;
}
@@ -43,8 +57,8 @@ export function usePlanMonitoring(config: Config) {
for (const file of mdFiles) {
const filePath = path.join(plansDir, file);
const stats = await fs.promises.stat(filePath);
if (stats.mtimeMs > latestMtime) {
const stats = await fs.promises.lstat(filePath);
if (stats.isFile() && stats.mtimeMs > latestMtime) {
latestMtime = stats.mtimeMs;
latestFile = file;
}
@@ -52,7 +66,7 @@ export function usePlanMonitoring(config: Config) {
if (
latestMtime > lastModifiedRef.current ||
latestFile !== planFileName
latestFile !== planFileNameRef.current
) {
const content = await fs.promises.readFile(
path.join(plansDir, latestFile),
@@ -61,6 +75,7 @@ export function usePlanMonitoring(config: Config) {
const todos = parseMarkdownTodos(content);
setPlanTodos(todos);
setPlanFileName(latestFile);
planFileNameRef.current = latestFile;
lastModifiedRef.current = latestMtime;
}
} catch (error) {
@@ -77,7 +92,7 @@ export function usePlanMonitoring(config: Config) {
}, 2000);
return () => clearInterval(interval);
}, [config, planFileName]);
}, [config]);
return { planTodos, planFileName };
}