mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-06-06 17:12:42 -07:00
fix(cli): address code review feedback for btw command
- Only register the `/btw` command if `experimental.btw` is enabled in settings.
- Explicitly clear the throttling timer in `useBtw.ts` when the display is dismissed to prevent delayed updates.
- Update the `/btw` placeholder example to use less technical language ("what does this function do?").
This commit is contained in:
@@ -991,6 +991,7 @@ export async function loadCliConfig(
|
||||
experimentalJitContext: settings.experimental?.jitContext,
|
||||
experimentalMemoryManager: settings.experimental?.memoryManager,
|
||||
contextManagement,
|
||||
experimentalBtw: settings.experimental?.btw,
|
||||
modelSteering: settings.experimental?.modelSteering,
|
||||
topicUpdateNarration: settings.experimental?.topicUpdateNarration,
|
||||
noBrowser: !!process.env['NO_BROWSER'],
|
||||
|
||||
@@ -165,6 +165,7 @@ describe('BuiltinCommandLoader', () => {
|
||||
getExtensionsEnabled: vi.fn().mockReturnValue(true),
|
||||
isSkillsSupportEnabled: vi.fn().mockReturnValue(true),
|
||||
isAgentsEnabled: vi.fn().mockReturnValue(false),
|
||||
isBtwEnabled: vi.fn().mockReturnValue(false),
|
||||
getMcpEnabled: vi.fn().mockReturnValue(true),
|
||||
getSkillManager: vi.fn().mockReturnValue({
|
||||
getAllSkills: vi.fn().mockReturnValue([]),
|
||||
@@ -301,6 +302,22 @@ describe('BuiltinCommandLoader', () => {
|
||||
expect(planCmd).toBeUndefined();
|
||||
});
|
||||
|
||||
it('should include btw command when btw is enabled', async () => {
|
||||
mockConfig.isBtwEnabled = vi.fn().mockReturnValue(true);
|
||||
const loader = new BuiltinCommandLoader(mockConfig);
|
||||
const commands = await loader.loadCommands(new AbortController().signal);
|
||||
const btwCmd = commands.find((c) => c.name === 'btw');
|
||||
expect(btwCmd).toBeDefined();
|
||||
});
|
||||
|
||||
it('should exclude btw command when btw is disabled', async () => {
|
||||
mockConfig.isBtwEnabled = vi.fn().mockReturnValue(false);
|
||||
const loader = new BuiltinCommandLoader(mockConfig);
|
||||
const commands = await loader.loadCommands(new AbortController().signal);
|
||||
const btwCmd = commands.find((c) => c.name === 'btw');
|
||||
expect(btwCmd).toBeUndefined();
|
||||
});
|
||||
|
||||
it('should exclude agents command when agents are disabled', async () => {
|
||||
mockConfig.isAgentsEnabled = vi.fn().mockReturnValue(false);
|
||||
const loader = new BuiltinCommandLoader(mockConfig);
|
||||
@@ -391,6 +408,7 @@ describe('BuiltinCommandLoader profile', () => {
|
||||
getExtensionsEnabled: vi.fn().mockReturnValue(true),
|
||||
isSkillsSupportEnabled: vi.fn().mockReturnValue(true),
|
||||
isAgentsEnabled: vi.fn().mockReturnValue(false),
|
||||
isBtwEnabled: vi.fn().mockReturnValue(false),
|
||||
getMcpEnabled: vi.fn().mockReturnValue(true),
|
||||
getSkillManager: vi.fn().mockReturnValue({
|
||||
getAllSkills: vi.fn().mockReturnValue([]),
|
||||
|
||||
@@ -121,7 +121,7 @@ export class BuiltinCommandLoader implements ICommandLoader {
|
||||
aboutCommand,
|
||||
...(this.config?.isAgentsEnabled() ? [agentsCommand] : []),
|
||||
authCommand,
|
||||
btwCommand,
|
||||
...(this.config?.isBtwEnabled() ? [btwCommand] : []),
|
||||
bugCommand,
|
||||
{
|
||||
...chatCommand,
|
||||
|
||||
@@ -54,7 +54,8 @@ describe('btwCommand', () => {
|
||||
expect(result).toEqual({
|
||||
type: 'message',
|
||||
messageType: 'error',
|
||||
content: 'Please provide a question, e.g. /btw what is this regex doing?',
|
||||
content:
|
||||
'Please provide a question, e.g. /btw what does this function do?',
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -29,7 +29,7 @@ export const btwCommand: SlashCommand = {
|
||||
type: 'message',
|
||||
messageType: 'error',
|
||||
content:
|
||||
'Please provide a question, e.g. /btw what is this regex doing?',
|
||||
'Please provide a question, e.g. /btw what does this function do?',
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -272,22 +272,36 @@ export const MainContent = () => {
|
||||
| {
|
||||
type: 'history';
|
||||
item: (typeof augmentedHistory)[0]['item'];
|
||||
element: React.ReactNode;
|
||||
isExpandable: boolean;
|
||||
isFirstThinking: boolean;
|
||||
isFirstAfterThinking: boolean;
|
||||
suppressNarration: boolean;
|
||||
}
|
||||
> = [
|
||||
{ type: 'header' as const },
|
||||
...augmentedHistory.map((data, index) => ({
|
||||
type: 'history' as const,
|
||||
item: data.item,
|
||||
element: historyItems[index],
|
||||
})),
|
||||
...augmentedHistory.map(
|
||||
({
|
||||
item,
|
||||
isExpandable,
|
||||
isFirstThinking,
|
||||
isFirstAfterThinking,
|
||||
suppressNarration,
|
||||
}) => ({
|
||||
type: 'history' as const,
|
||||
item,
|
||||
isExpandable,
|
||||
isFirstThinking,
|
||||
isFirstAfterThinking,
|
||||
suppressNarration,
|
||||
}),
|
||||
),
|
||||
{ type: 'pending' as const },
|
||||
];
|
||||
if (uiState.btwState.isActive) {
|
||||
data.push({ type: 'btw' as const });
|
||||
}
|
||||
return data;
|
||||
}, [augmentedHistory, historyItems, uiState.btwState.isActive]);
|
||||
}, [augmentedHistory, uiState.btwState.isActive]);
|
||||
|
||||
const renderItem = useCallback(
|
||||
({ item }: { item: (typeof virtualizedData)[number] }) => {
|
||||
@@ -300,8 +314,25 @@ export const MainContent = () => {
|
||||
/>
|
||||
);
|
||||
} else if (item.type === 'history') {
|
||||
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
|
||||
return (item as any).element;
|
||||
return (
|
||||
<MemoizedHistoryItemDisplay
|
||||
terminalWidth={mainAreaWidth}
|
||||
availableTerminalHeight={
|
||||
uiState.constrainHeight || !item.isExpandable
|
||||
? staticAreaMaxItemHeight
|
||||
: undefined
|
||||
}
|
||||
availableTerminalHeightGemini={MAX_GEMINI_MESSAGE_LINES}
|
||||
key={item.item.id}
|
||||
item={item.item}
|
||||
isPending={false}
|
||||
commands={uiState.slashCommands}
|
||||
isExpandable={item.isExpandable}
|
||||
isFirstThinking={item.isFirstThinking}
|
||||
isFirstAfterThinking={item.isFirstAfterThinking}
|
||||
suppressNarration={item.suppressNarration}
|
||||
/>
|
||||
);
|
||||
} else if (item.type === 'btw') {
|
||||
return <>{btwDisplayNode}</>;
|
||||
} else {
|
||||
@@ -311,7 +342,11 @@ export const MainContent = () => {
|
||||
[
|
||||
showHeaderDetails,
|
||||
version,
|
||||
mainAreaWidth,
|
||||
uiState.slashCommands,
|
||||
pendingItems,
|
||||
uiState.constrainHeight,
|
||||
staticAreaMaxItemHeight,
|
||||
btwDisplayNode,
|
||||
],
|
||||
);
|
||||
|
||||
@@ -190,7 +190,7 @@ describe('useBtw', () => {
|
||||
// wait for the catch/finally blocks inside submitBtw to finish
|
||||
try {
|
||||
await submitPromise;
|
||||
} catch (_e) {
|
||||
} catch {
|
||||
// ignore AbortError
|
||||
}
|
||||
});
|
||||
|
||||
@@ -81,12 +81,17 @@ export const useBtw = (
|
||||
|
||||
const abortControllerRef = useRef<AbortController | null>(null);
|
||||
const requestIdRef = useRef<number>(0);
|
||||
const flushTimerRef = useRef<NodeJS.Timeout | null>(null);
|
||||
|
||||
const dismissBtw = useCallback(() => {
|
||||
if (abortControllerRef.current) {
|
||||
abortControllerRef.current.abort();
|
||||
abortControllerRef.current = null;
|
||||
}
|
||||
if (flushTimerRef.current) {
|
||||
clearTimeout(flushTimerRef.current);
|
||||
flushTimerRef.current = null;
|
||||
}
|
||||
requestIdRef.current++;
|
||||
dispatch({ type: 'DISMISS' });
|
||||
}, []);
|
||||
@@ -108,7 +113,6 @@ export const useBtw = (
|
||||
|
||||
let accumulatedResponse = '';
|
||||
let lastDispatchTime = 0;
|
||||
let flushTimer: NodeJS.Timeout | null = null;
|
||||
|
||||
const flushResponse = () => {
|
||||
if (requestIdRef.current !== requestId) return;
|
||||
@@ -130,22 +134,21 @@ export const useBtw = (
|
||||
case GeminiEventType.Content: {
|
||||
accumulatedResponse += event.value ?? '';
|
||||
const now = Date.now();
|
||||
if (now - lastDispatchTime > 50) {
|
||||
if (flushTimer) {
|
||||
clearTimeout(flushTimer);
|
||||
flushTimer = null;
|
||||
}
|
||||
flushResponse();
|
||||
} else if (!flushTimer) {
|
||||
flushTimer = setTimeout(() => {
|
||||
if (!flushTimerRef.current) {
|
||||
const timeSinceLastDispatch = now - lastDispatchTime;
|
||||
if (timeSinceLastDispatch >= 50) {
|
||||
flushResponse();
|
||||
flushTimer = null;
|
||||
}, 50);
|
||||
} else {
|
||||
flushTimerRef.current = setTimeout(() => {
|
||||
flushResponse();
|
||||
flushTimerRef.current = null;
|
||||
}, 50 - timeSinceLastDispatch);
|
||||
}
|
||||
}
|
||||
break;
|
||||
}
|
||||
case GeminiEventType.Error: {
|
||||
if (flushTimer) clearTimeout(flushTimer);
|
||||
if (flushTimerRef.current) clearTimeout(flushTimerRef.current);
|
||||
flushResponse();
|
||||
|
||||
const value = event.value;
|
||||
@@ -174,7 +177,7 @@ export const useBtw = (
|
||||
}
|
||||
case GeminiEventType.Finished:
|
||||
case GeminiEventType.UserCancelled:
|
||||
if (flushTimer) clearTimeout(flushTimer);
|
||||
if (flushTimerRef.current) clearTimeout(flushTimerRef.current);
|
||||
flushResponse();
|
||||
dispatch({ type: 'FINISHED' });
|
||||
break;
|
||||
@@ -183,7 +186,7 @@ export const useBtw = (
|
||||
}
|
||||
}
|
||||
} catch (err) {
|
||||
if (flushTimer) clearTimeout(flushTimer);
|
||||
if (flushTimerRef.current) clearTimeout(flushTimerRef.current);
|
||||
flushResponse();
|
||||
|
||||
if (err instanceof Error && err.name === 'AbortError') {
|
||||
@@ -195,7 +198,7 @@ export const useBtw = (
|
||||
});
|
||||
}
|
||||
} finally {
|
||||
if (flushTimer) clearTimeout(flushTimer);
|
||||
if (flushTimerRef.current) clearTimeout(flushTimerRef.current);
|
||||
flushResponse();
|
||||
|
||||
if (requestIdRef.current === requestId) {
|
||||
|
||||
@@ -3067,6 +3067,21 @@ describe('Config Quota & Preview Model Access', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('isBtwEnabled', () => {
|
||||
it('should return false for isBtwEnabled by default', () => {
|
||||
const config = new Config(baseParams);
|
||||
expect(config.isBtwEnabled()).toBe(false);
|
||||
});
|
||||
|
||||
it('should return true for isBtwEnabled when experimentalBtw is true', () => {
|
||||
const config = new Config({
|
||||
...baseParams,
|
||||
experimentalBtw: true,
|
||||
});
|
||||
expect(config.isBtwEnabled()).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('getPlanModeRoutingEnabled', () => {
|
||||
it('should default to true when not provided', async () => {
|
||||
const config = new Config(baseParams);
|
||||
|
||||
@@ -704,6 +704,7 @@ export interface ConfigParameters {
|
||||
experimentalAgentHistoryTruncationThreshold?: number;
|
||||
experimentalAgentHistoryRetainedMessages?: number;
|
||||
experimentalAgentHistorySummarization?: boolean;
|
||||
experimentalBtw?: boolean;
|
||||
memoryBoundaryMarkers?: string[];
|
||||
topicUpdateNarration?: boolean;
|
||||
|
||||
@@ -942,6 +943,7 @@ export class Config implements McpContext, AgentLoopContext {
|
||||
private readonly adminSkillsEnabled: boolean;
|
||||
private readonly experimentalJitContext: boolean;
|
||||
private readonly experimentalMemoryManager: boolean;
|
||||
private readonly experimentalBtw: boolean;
|
||||
private readonly memoryBoundaryMarkers: readonly string[];
|
||||
private readonly topicUpdateNarration: boolean;
|
||||
private readonly disableLLMCorrection: boolean;
|
||||
@@ -1153,6 +1155,7 @@ export class Config implements McpContext, AgentLoopContext {
|
||||
|
||||
this.experimentalJitContext = params.experimentalJitContext ?? false;
|
||||
this.experimentalMemoryManager = params.experimentalMemoryManager ?? false;
|
||||
this.experimentalBtw = params.experimentalBtw ?? false;
|
||||
this.memoryBoundaryMarkers = params.memoryBoundaryMarkers ?? ['.git'];
|
||||
this.contextManagement = {
|
||||
enabled: params.contextManagement?.enabled ?? false,
|
||||
@@ -2859,6 +2862,10 @@ export class Config implements McpContext, AgentLoopContext {
|
||||
return this.planEnabled;
|
||||
}
|
||||
|
||||
isBtwEnabled(): boolean {
|
||||
return this.experimentalBtw;
|
||||
}
|
||||
|
||||
isTrackerEnabled(): boolean {
|
||||
return this.trackerEnabled;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user