From cb68095627d6ddf8d7b879f4eb3d4747aa4b71ff Mon Sep 17 00:00:00 2001 From: Jarrod Whelan <150866123+jwhelangoog@users.noreply.github.com> Date: Tue, 7 Apr 2026 02:45:22 -0700 Subject: [PATCH] fix(cli): ensure consistent tool top-borders during sequences of tool outputs (#24513) Fixes a visual regression where tool outputs could render without a top border ("capless") when multiple tools completed at different times or during transient pending states. The issue was caused by a synchronization failure between how history items are "pushed" (from pending to committed items) and how their borders were determined. Changes: - Ensured `pendingToolGroupItems` always uses `borderTop: true` to avoid transient capless state for executing tools. - Removed redundant `isFirstInThisPush` flag. - Added `useGeminiStream.BorderRendering.test.tsx` to permanently cover sequential tool completion and pending border states. --- .../useGeminiStream.BorderRendering.test.tsx | 194 ++++++++++++++++++ packages/cli/src/ui/hooks/useGeminiStream.ts | 43 ++-- 2 files changed, 208 insertions(+), 29 deletions(-) create mode 100644 packages/cli/src/ui/hooks/useGeminiStream.BorderRendering.test.tsx diff --git a/packages/cli/src/ui/hooks/useGeminiStream.BorderRendering.test.tsx b/packages/cli/src/ui/hooks/useGeminiStream.BorderRendering.test.tsx new file mode 100644 index 0000000000..cafc30032c --- /dev/null +++ b/packages/cli/src/ui/hooks/useGeminiStream.BorderRendering.test.tsx @@ -0,0 +1,194 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach, type Mock } from 'vitest'; +import { act } from 'react'; +import { useGeminiStream } from './useGeminiStream.js'; +import { renderHookWithProviders } from '../../test-utils/render.js'; +import { waitFor } from '../../test-utils/async.js'; +import { + CoreToolCallStatus, + Kind, + type Config, + type AnyToolInvocation, +} from '@google/gemini-cli-core'; +import type { + TrackedToolCall, + TrackedCompletedToolCall, + TrackedExecutingToolCall, +} from './useToolScheduler.js'; +import type { LoadedSettings } from '../../config/settings.js'; + +// Mock useToolScheduler +const mockScheduleToolCalls = vi.fn(); +const mockCancelAllToolCalls = vi.fn(); +const mockMarkToolsAsSubmitted = vi.fn(); + +vi.mock('./useToolScheduler.js', () => ({ + useToolScheduler: vi.fn(), +})); + +// Mock other hooks +vi.mock('./useAlternateBuffer.js', () => ({ + useAlternateBuffer: vi.fn(() => false), +})); + +describe('useGeminiStream Border Logic', () => { + let mockAddItem = vi.fn(); + const mockConfig = { + getApprovalMode: vi.fn(() => 'AUTO_EDIT'), + getGeminiClient: vi.fn(() => ({ + getChat: vi.fn(() => ({ + recordCompletedToolCalls: vi.fn(), + })), + getCurrentSequenceModel: vi.fn(), + })), + getModel: vi.fn(() => 'gemini-2.0-flash'), + getProjectRoot: vi.fn(), + getCheckpointingEnabled: vi.fn(() => false), + getContentGeneratorConfig: vi.fn(() => ({ authType: 'none' })), + storage: {}, + } as unknown as Config; + + const mockLoadedSettings = { + merged: { + ui: { + compactToolOutput: true, + }, + }, + } as LoadedSettings; + + beforeEach(async () => { + vi.clearAllMocks(); + mockAddItem = vi.fn(); + const { useToolScheduler } = await import('./useToolScheduler.js'); + (useToolScheduler as Mock).mockReturnValue([ + [], + mockScheduleToolCalls, + mockMarkToolsAsSubmitted, + vi.fn(), + mockCancelAllToolCalls, + 0, + ]); + }); + + it('should maintain borderTop when standard tools are pushed in separate batches (Fixes #24513)', async () => { + let currentToolCalls: TrackedToolCall[] = []; + + const { useToolScheduler } = await import('./useToolScheduler.js'); + (useToolScheduler as Mock).mockImplementation(() => [ + currentToolCalls, + mockScheduleToolCalls, + mockMarkToolsAsSubmitted, + vi.fn(), + mockCancelAllToolCalls, + 0, + ]); + + const tool1: TrackedToolCall = { + request: { + callId: 'call1', + name: 'shell', + args: {}, + prompt_id: 'p1', + isClientInitiated: false, + }, + status: CoreToolCallStatus.Success, + response: { + callId: 'call1', + responseParts: [], + resultDisplay: 'out1', + error: undefined, + errorType: undefined, + }, + tool: { kind: Kind.Execute, displayName: 'shell' }, + invocation: { getDescription: () => 'desc1' } as AnyToolInvocation, + responseSubmittedToGemini: false, + } as unknown as TrackedCompletedToolCall; + + const tool2Executing: TrackedToolCall = { + request: { + callId: 'call2', + name: 'shell', + args: {}, + prompt_id: 'p1', + isClientInitiated: false, + }, + status: CoreToolCallStatus.Executing, + tool: { kind: Kind.Execute, displayName: 'shell' }, + invocation: { getDescription: () => 'desc2' } as AnyToolInvocation, + } as unknown as TrackedExecutingToolCall; + + const tool2Success: TrackedToolCall = { + ...tool2Executing, + status: CoreToolCallStatus.Success, + response: { + callId: 'call2', + responseParts: [], + resultDisplay: 'out2', + error: undefined, + errorType: undefined, + }, + responseSubmittedToGemini: false, + } as unknown as TrackedCompletedToolCall; + + currentToolCalls = [tool1, tool2Executing]; + + const { result, rerender } = await renderHookWithProviders(() => + useGeminiStream( + mockConfig.getGeminiClient(), + [], + mockAddItem, + mockConfig, + mockLoadedSettings, + vi.fn(), + vi.fn().mockResolvedValue(false), + false, + () => undefined, + vi.fn(), + vi.fn(), + false, + vi.fn(), + vi.fn(), + vi.fn(), + 80, + 24, + ), + ); + + // Initial render should push Tool 1 (since it's success) + await waitFor(() => { + expect(mockAddItem).toHaveBeenCalledTimes(1); + }); + + // Check pending items - Tool 2 is executing + expect(result.current.pendingHistoryItems.length).toBeGreaterThan(0); + const pendingItem = result.current.pendingHistoryItems.find( + (i) => i.type === 'tool_group', + ); + expect(pendingItem).toBeDefined(); + // This is expected to FAIL currently (it will be false) + expect(pendingItem?.borderTop).toBe(true); + + // Now Tool 2 completes + currentToolCalls = [tool1, tool2Success]; + await act(async () => { + rerender(); + }); + + // Second push for Tool 2 + await waitFor(() => { + expect(mockAddItem).toHaveBeenCalledTimes(2); + }); + + const call1Item = mockAddItem.mock.calls[0][0]; + const call2Item = mockAddItem.mock.calls[1][0]; + + // Both should have borderTop: true because they were pushed in separate history items + expect(call1Item.borderTop).toBe(true); + expect(call2Item.borderTop).toBe(true); + }); +}); diff --git a/packages/cli/src/ui/hooks/useGeminiStream.ts b/packages/cli/src/ui/hooks/useGeminiStream.ts index a2621c4546..bfaabcf5a6 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.ts +++ b/packages/cli/src/ui/hooks/useGeminiStream.ts @@ -306,10 +306,9 @@ export const useGeminiStream = ( const tcIndex = toolCalls.indexOf(firstToolToPush); const prevTool = tcIndex > 0 ? toolCalls[tcIndex - 1] : null; - let borderTop = isFirstToolInGroupRef.current; - if (!borderTop && prevTool) { - // If the first tool in this push is non-compact but follows a compact tool, - // we must start a new border group. + // Start a new box for a new history item + let borderTop = true; + if (prevTool) { const currentIsCompact = isCompactTool( mapTrackedToolCallsToDisplay(firstToolToPush).tools[0], isCompactModeEnabled, @@ -318,8 +317,10 @@ export const useGeminiStream = ( mapTrackedToolCallsToDisplay(prevTool).tools[0], isCompactModeEnabled, ); - if (!currentIsCompact && prevWasCompact) { - borderTop = true; + + // Continue the existing border group for consecutive non-compact tools + if (!currentIsCompact && !prevWasCompact) { + borderTop = false; } } @@ -490,7 +491,6 @@ export const useGeminiStream = ( if (toolsToPush.length > 0) { const newPushed = new Set(pushedToolCallIdsRef.current); - const isFirstInThisPush = isFirstToolInGroupRef.current; const isCompactModeEnabled = settings.merged.ui?.compactToolOutput === true; @@ -516,19 +516,12 @@ export const useGeminiStream = ( for (let i = 0; i < groups.length; i++) { const group = groups[i]; - const isFirstInBatch = i === 0 && isFirstInThisPush; - const lastTcInGroup = group[group.length - 1]; - const tcIndexInBatch = toolCalls.indexOf(lastTcInGroup); - const isLastInBatch = tcIndexInBatch === toolCalls.length - 1; + const isFirstInBatch = i === 0; // Each new history item starts its own border group + const isLastInBatch = i === groups.length - 1; // Each history item closes its own border group - const nextTcInBatch = - tcIndexInBatch < toolCalls.length - 1 - ? toolCalls[tcIndexInBatch + 1] - : null; + const nextTcInBatch = i < groups.length - 1 ? groups[i + 1][0] : null; const prevTcInBatch = - toolCalls.indexOf(group[0]) > 0 - ? toolCalls[toolCalls.indexOf(group[0]) - 1] - : null; + i > 0 ? groups[i - 1][groups[i - 1].length - 1] : null; const historyItem = mapTrackedToolCallsToDisplay(group, { ...getToolGroupBorderAppearance( @@ -611,17 +604,9 @@ export const useGeminiStream = ( ); if (remainingTools.length > 0) { - // Should we draw a top border? Yes if NO previous tools were drawn, - // OR if ALL previously drawn tools were topics (which don't draw top borders). - let needsTopBorder = pushedToolCallIds.size === 0; - if (!needsTopBorder) { - const allPushedWereTopics = toolCalls - .filter((tc) => pushedToolCallIds.has(tc.request.callId)) - .every((tc) => isTopicTool(tc.request.name)); - if (allPushedWereTopics) { - needsTopBorder = true; - } - } + // The pending group always needs a top border because any previously + // pushed tool history items have already closed their own border boxes. + const needsTopBorder = true; items.push( mapTrackedToolCallsToDisplay(remainingTools, {