Code review comment fixes.

This commit is contained in:
Jacob Richman
2026-05-18 12:34:11 -07:00
parent 1828fafb25
commit e5818586b8
9 changed files with 146 additions and 19 deletions
@@ -565,6 +565,45 @@ describe('DenseToolMessage', () => {
expect(lastFrame()).toContain('new line');
});
it('shows diff content when globally expanded inside a VirtualizedList context', async () => {
const mockListContext = {
registerInteractivity: vi.fn(),
setItemState: vi.fn(),
getItemState: vi.fn(),
isItemToggled: vi.fn().mockReturnValue(false),
toggleItem: vi.fn(),
registerClickCallback: vi.fn(),
unregisterClickCallback: vi.fn(),
};
const { lastFrame, waitUntilReady } = await renderWithProviders(
<VirtualizedListContext.Provider
value={
mockListContext as unknown as React.ContextType<
typeof VirtualizedListContext
>
}
>
<DenseToolMessage
{...defaultProps}
itemKey="item-1"
resultDisplay={diffResult as ToolResultDisplay}
status={CoreToolCallStatus.Success}
/>
</VirtualizedListContext.Provider>,
{
config: makeFakeConfig({ useAlternateBuffer: true }),
settings: createMockSettings({ ui: { useAlternateBuffer: true } }),
toolActions: {
isExpanded: () => true,
},
},
);
await waitUntilReady();
expect(lastFrame()).toContain('new line');
});
it('toggles expansion when header is clicked', async () => {
const toggleExpansion = vi.fn();
const toggleItem = vi.fn();
@@ -283,10 +283,15 @@ export const DenseToolMessage: React.FC<DenseToolMessageProps> = (props) => {
// Determine expansion state based on list context or fallback to tool actions
const isExpanded = useMemo(() => {
const isExpandedGlobally = isExpandedInContext
? isExpandedInContext(callId)
: false;
if (itemKey && virtualizedListContext) {
return virtualizedListContext.isItemToggled(itemKey);
return (
virtualizedListContext.isItemToggled(itemKey) || isExpandedGlobally
);
}
return isExpandedInContext ? isExpandedInContext(callId) : false;
return isExpandedGlobally;
}, [itemKey, virtualizedListContext, isExpandedInContext, callId]);
const handleToggle = useCallback(() => {
@@ -24,6 +24,7 @@ interface TopicMessageProps extends IndividualToolCallDisplay {
terminalWidth: number;
availableTerminalHeight?: number;
isExpandable?: boolean;
// TopicMessage is only interactive when rendered inside VirtualizedList.
itemKey?: string;
}
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2025 Google LLC
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
@@ -0,0 +1,55 @@
/**
* @license
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
import { act } from 'react';
import { Box, Text } from 'ink';
import { describe, expect, it } from 'vitest';
import { renderWithProviders as render } from '../../../test-utils/render.js';
import {
FixedVirtualizedList,
SCROLL_TO_ITEM_END,
} from './FixedVirtualizedList.js';
describe('<FixedVirtualizedList />', () => {
const renderList = (data: string[]) => (
<Box height={5} width={80}>
<FixedVirtualizedList
data={data}
renderItem={({ item }) => (
<Box height={1}>
<Text>{item}</Text>
</Box>
)}
itemHeight={1}
keyExtractor={(item) => item}
initialScrollIndex={SCROLL_TO_ITEM_END}
initialScrollOffsetInIndex={SCROLL_TO_ITEM_END}
width={80}
maxHeight={5}
/>
</Box>
);
it('sticks to the bottom when data grows', async () => {
const initialData = Array.from({ length: 10 }, (_, i) => `Item ${i}`);
const { lastFrame, rerender, waitUntilReady, unmount } = await render(
renderList(initialData),
);
await waitUntilReady();
expect(lastFrame()).toContain('Item 9');
const newData = [...initialData, 'Item 10', 'Item 11'];
await act(async () => {
rerender(renderList(newData));
});
await waitUntilReady();
expect(lastFrame()).toContain('Item 11');
expect(lastFrame()).not.toContain('Item 0');
unmount();
});
});
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2025 Google LLC
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
@@ -172,23 +172,21 @@ function FixedVirtualizedList<T>(
props.targetScrollIndex,
);
const prevDataLength = useRef(data.length);
const previousDataLength = prevDataLength.current;
if (
(props.targetScrollIndex !== undefined &&
props.targetScrollIndex !== prevTargetScrollIndex &&
data.length > 0) ||
(props.targetScrollIndex !== undefined &&
prevDataLength.current === 0 &&
previousDataLength === 0 &&
data.length > 0)
) {
if (props.targetScrollIndex !== prevTargetScrollIndex) {
setPrevTargetScrollIndex(props.targetScrollIndex);
}
prevDataLength.current = data.length;
setIsStickingToBottom(false);
setScrollAnchor({ index: props.targetScrollIndex, offset: 0 });
} else {
prevDataLength.current = data.length;
}
const rawStateActualScrollTop = (() => {
@@ -236,7 +234,7 @@ function FixedVirtualizedList<T>(
}
}
const listGrew = data.length > prevDataLength.current;
const listGrew = data.length > previousDataLength;
const containerChanged =
prevContainerHeight.current !== scrollableContainerHeight;
const shouldAutoScroll = props.targetScrollIndex === undefined;
@@ -119,6 +119,41 @@ describe('<VirtualizedList />', () => {
unmount();
});
it('rerenders cached items when renderItem changes', async () => {
const data = ['Item 0'];
const renderWithLabel = (label: string) => (
<Box height={10} width={100}>
<VirtualizedList
data={data}
renderItem={({ item }) => (
<Box height={1}>
<Text>
{label} {item}
</Text>
</Box>
)}
keyExtractor={keyExtractor}
estimatedItemHeight={() => itemHeight}
/>
</Box>
);
const { lastFrame, rerender, waitUntilReady, unmount } = await render(
renderWithLabel('Initial'),
);
await waitUntilReady();
expect(lastFrame()).toContain('Initial Item 0');
await act(async () => {
rerender(renderWithLabel('Updated'));
});
await waitUntilReady();
expect(lastFrame()).toContain('Updated Item 0');
expect(lastFrame()).not.toContain('Initial Item 0');
unmount();
});
it('scrolls down to show new items when requested via ref', async () => {
const ref = createRef<VirtualizedListRef<string>>();
const { lastFrame, waitUntilReady, unmount } = await render(
@@ -926,6 +926,7 @@ function VirtualizedList<T>(
containerWidth: number;
index: number;
isToggled: boolean;
renderItem: typeof renderItem;
}
>(),
);
@@ -965,7 +966,8 @@ function VirtualizedList<T>(
cached.width === width &&
cached.containerWidth === containerWidth &&
cached.index === i &&
cached.isToggled === isToggled
cached.isToggled === isToggled &&
cached.renderItem === renderItem
) {
contentElement = cached.element;
} else {
@@ -998,6 +1000,7 @@ function VirtualizedList<T>(
containerWidth,
index: i,
isToggled,
renderItem,
});
}
@@ -12,7 +12,6 @@ import {
type MouseEvent,
type MouseEventName,
} from '../contexts/MouseContext.js';
import { debugLogger } from '@google/gemini-cli-core';
export const useMouseClick = (
containerRef: React.RefObject<DOMElement | null>,
@@ -32,9 +31,6 @@ export const useMouseClick = (
const eventName =
name ?? (button === 'left' ? 'left-press' : 'right-release');
debugLogger.log(
`[useMouseClick] received event=${event.name} expected=${eventName} hasContainer=${!!containerRef.current}`,
);
if (event.name === eventName && containerRef.current) {
const { x, y, width, height } = getBoundingBox(containerRef.current);
// Terminal mouse events are 1-based, Ink layout is 0-based.
@@ -44,17 +40,12 @@ export const useMouseClick = (
const relativeX = mouseX - x;
const relativeY = mouseY - y;
debugLogger.log(
`[useMouseClick] bounds x=${x} y=${y} w=${width} h=${height} mouseX=${mouseX} mouseY=${mouseY} relX=${relativeX} relY=${relativeY}`,
);
if (
relativeX >= 0 &&
relativeX < width &&
relativeY >= 0 &&
relativeY < height
) {
debugLogger.log(`[useMouseClick] Triggering handler!`);
handlerRef.current(event, relativeX, relativeY);
}
}