fix: address PR feedback for slash commands and background task tracking

This commit is contained in:
jacob314
2026-05-18 12:40:15 -07:00
parent 3e1be515e5
commit d78db4f514
5 changed files with 73 additions and 84 deletions
+32 -34
View File
@@ -1180,40 +1180,38 @@ Logging in with Google... Restarting Gemini CLI to continue.
[config, getPreferredEditor],
);
const agentStream = useAgentStream({
agent: streamAgent,
addItem: historyManager.addItem,
handleSlashCommand,
onCancelSubmit,
isShellFocused: embeddedShellFocused,
logger,
isActive: !!streamAgent,
});
const geminiStream = useGeminiStream(
config.getGeminiClient(),
historyManager.history,
historyManager.addItem,
config,
settings,
setDebugMessage,
handleSlashCommand,
shellModeActive,
getPreferredEditor,
onAuthError,
performMemoryRefresh,
modelSwitchedFromQuotaError,
setModelSwitchedFromQuotaError,
onCancelSubmit,
setEmbeddedShellFocused,
terminalWidth,
terminalHeight,
embeddedShellFocused,
consumePendingHints,
!streamAgent,
);
const activeStream = streamAgent ? agentStream : geminiStream;
const activeStream = streamAgent
? // eslint-disable-next-line react-hooks/rules-of-hooks
useAgentStream({
agent: streamAgent,
addItem: historyManager.addItem,
handleSlashCommand,
onCancelSubmit,
isShellFocused: embeddedShellFocused,
logger,
})
: // eslint-disable-next-line react-hooks/rules-of-hooks
useGeminiStream(
config.getGeminiClient(),
historyManager.history,
historyManager.addItem,
config,
settings,
setDebugMessage,
handleSlashCommand,
shellModeActive,
getPreferredEditor,
onAuthError,
performMemoryRefresh,
modelSwitchedFromQuotaError,
setModelSwitchedFromQuotaError,
onCancelSubmit,
setEmbeddedShellFocused,
terminalWidth,
terminalHeight,
embeddedShellFocused,
consumePendingHints,
);
const {
streamingState,
@@ -128,6 +128,10 @@ describe('useAgentStream', () => {
expect(mockAgentProtocol.send).toHaveBeenCalledWith({
message: { content: [{ type: 'text', text: 'modified prompt' }] },
});
expect(mockAddItem).toHaveBeenCalledWith(
expect.objectContaining({ type: MessageType.USER, text: '/mcp-prompt' }),
expect.any(Number),
);
});
it('should update streamingState based on agent_start and agent_end events', async () => {
+17 -20
View File
@@ -53,7 +53,6 @@ export interface UseAgentStreamOptions {
) => void;
isShellFocused?: boolean;
logger?: Logger | null;
isActive?: boolean;
}
/**
@@ -67,7 +66,6 @@ export const useAgentStream = ({
onCancelSubmit,
isShellFocused,
logger,
isActive = true,
}: UseAgentStreamOptions) => {
const [initError] = useState<string | null>(null);
const [retryStatus] = useState<RetryAttemptPayload | null>(null);
@@ -336,24 +334,22 @@ export const useAgentStream = ({
);
useEffect(() => {
if (!isActive) return;
const unsubscribe = agent?.subscribe(handleEvent);
return () => unsubscribe?.();
}, [agent, handleEvent, isActive]);
}, [agent, handleEvent]);
useKeypress(
(key) => {
if (key.name === 'escape' && !isShellFocused) {
void cancelOngoingRequest(false);
if (key.ctrl && key.name === 'c') {
void cancelOngoingRequest();
return true;
}
return false;
},
{
isActive:
isActive &&
(streamingState === StreamingState.Responding ||
streamingState === StreamingState.WaitingForConfirmation),
streamingState === StreamingState.Responding ||
streamingState === StreamingState.WaitingForConfirmation,
},
);
@@ -403,14 +399,18 @@ export const useAgentStream = ({
}
if (shouldAddToHistory) {
const queryText =
typeof localQuery === 'string'
? localQuery
: partToString(localQuery);
const originalQueryText =
typeof query === 'string' ? query : partToString(query);
addItem({ type: MessageType.USER, text: queryText }, timestamp);
addItem(
{ type: MessageType.USER, text: originalQueryText },
timestamp,
);
if (typeof localQuery !== 'string') {
void logger?.logMessage(MessageSenderType.USER, queryText);
void logger?.logMessage(
MessageSenderType.USER,
partToString(localQuery),
);
}
}
startNewPrompt();
@@ -436,9 +436,7 @@ export const useAgentStream = ({
},
[agent, addItem, logger, startNewPrompt, handleSlashCommand],
);
useEffect(() => {
if (!isActive) return;
if (trackedTools.length > 0) {
const isNewBatch = !trackedTools.some((tc) =>
pushedToolCallIdsRef.current.has(tc.callId),
@@ -448,6 +446,7 @@ export const useAgentStream = ({
setIsFirstToolInGroup(true);
}
} else if (streamingState === StreamingState.Idle) {
// Clear when idle to be ready for next turn
setPushedToolCallIds(new Set());
setIsFirstToolInGroup(true);
}
@@ -457,12 +456,11 @@ export const useAgentStream = ({
setPushedToolCallIds,
setIsFirstToolInGroup,
streamingState,
isActive,
]);
// Push completed tools to history
useEffect(() => {
if (!isActive || trackedTools.length === 0) return;
if (trackedTools.length === 0) return;
// We only push to history once all currently known tools in the turn are terminal.
// This allows ToolGroupDisplay to correctly hoist ALL notices (topics) for the turn.
@@ -532,7 +530,6 @@ export const useAgentStream = ({
activePtyId,
isShellFocused,
backgroundTasks,
isActive,
]);
const pendingToolGroupItems = useMemo((): HistoryItemWithoutId[] => {
@@ -86,7 +86,6 @@ export const useExecutionLifecycle = (
terminalHeight?: number,
activeBackgroundExecutionId?: number,
isWaitingForConfirmation?: boolean,
isActive: boolean = true,
) => {
const [state, dispatch] = useReducer(shellReducer, initialState);
@@ -112,7 +111,6 @@ export const useExecutionLifecycle = (
state.activeShellPtyId ?? activeBackgroundExecutionId ?? undefined;
useEffect(() => {
if (!isActive) return;
const isForegroundActive = !!activePtyId || !!isWaitingForConfirmation;
if (isForegroundActive) {
@@ -146,7 +144,6 @@ export const useExecutionLifecycle = (
state.isBackgroundTaskVisible,
m,
dispatch,
isActive,
]);
useEffect(
@@ -161,7 +158,6 @@ export const useExecutionLifecycle = (
);
const toggleBackgroundTasks = useCallback(() => {
if (!isActive) return;
if (state.backgroundTasks.size > 0) {
const willBeVisible = !state.isBackgroundTaskVisible;
dispatch({ type: 'TOGGLE_VISIBILITY' });
@@ -197,11 +193,9 @@ export const useExecutionLifecycle = (
isWaitingForConfirmation,
m,
dispatch,
isActive,
]);
const backgroundCurrentExecution = useCallback(() => {
if (!isActive) return;
const pidToBackground =
state.activeShellPtyId ?? activeBackgroundExecutionId;
if (pidToBackground) {
@@ -224,11 +218,10 @@ export const useExecutionLifecycle = (
m.restoreTimeout = null;
}
}
}, [state.activeShellPtyId, activeBackgroundExecutionId, m, isActive]);
}, [state.activeShellPtyId, activeBackgroundExecutionId, m]);
const dismissBackgroundTask = useCallback(
async (pid: number) => {
if (!isActive) return;
const shell = state.backgroundTasks.get(pid);
if (shell) {
if (shell.status === 'running') {
@@ -247,7 +240,7 @@ export const useExecutionLifecycle = (
}
}
},
[state.backgroundTasks, dispatch, m, isActive],
[state.backgroundTasks, dispatch, m],
);
const registerBackgroundTask = useCallback(
@@ -257,7 +250,6 @@ export const useExecutionLifecycle = (
initialOutput: string | AnsiOutput,
completionBehavior?: CompletionBehavior,
) => {
if (!isActive) return;
m.backgroundedPids.add(pid);
dispatch({
type: 'REGISTER_TASK',
@@ -321,7 +313,7 @@ export const useExecutionLifecycle = (
dataUnsubscribe();
});
},
[dispatch, m, isActive],
[dispatch, m],
);
// Auto-register any execution that gets backgrounded, regardless of type.
+17 -19
View File
@@ -238,7 +238,6 @@ export const useGeminiStream = (
terminalHeight: number,
isShellFocused?: boolean,
consumeUserHint?: () => string | null,
isActive: boolean = true,
) => {
const [initError, setInitError] = useState<string | null>(null);
const [retryStatus, setRetryStatus] = useState<RetryAttemptPayload | null>(
@@ -277,7 +276,6 @@ export const useGeminiStream = (
}, [config]);
useEffect(() => {
if (!isActive) return;
const handleRetryAttempt = (payload: RetryAttemptPayload) => {
if (turnCancelledRef.current || !isRespondingRef.current) {
return;
@@ -288,7 +286,7 @@ export const useGeminiStream = (
return () => {
coreEvents.off(CoreEvent.RetryAttempt, handleRetryAttempt);
};
}, [isRespondingRef, isActive]);
}, [isRespondingRef]);
const [
toolCalls,
@@ -417,12 +415,10 @@ export const useGeminiStream = (
terminalHeight,
activeBackgroundExecutionId,
streamingState === StreamingState.WaitingForConfirmation,
isActive,
);
// Reset tracking when a new batch of tools starts
useEffect(() => {
if (!isActive) return;
if (toolCalls.length > 0) {
const isNewBatch = !toolCalls.some((tc) =>
pushedToolCallIdsRef.current.has(tc.request.callId),
@@ -442,12 +438,10 @@ export const useGeminiStream = (
setPushedToolCallIds,
setIsFirstToolInGroup,
streamingState,
isActive,
]);
// Push completed tools to history as they finish
useEffect(() => {
if (!isActive) return;
const toolsToPush: TrackedToolCall[] = [];
for (let i = 0; i < toolCalls.length; i++) {
const tc = toolCalls[i];
@@ -605,11 +599,9 @@ export const useGeminiStream = (
isShellFocused,
backgroundTasks,
settings.merged.ui?.compactToolOutput,
isActive,
]);
const pendingToolGroupItems = useMemo((): HistoryItemWithoutId[] => {
if (!isActive) return [];
const remainingTools = toolCalls.filter(
(tc) => !pushedToolCallIds.has(tc.request.callId),
);
@@ -714,7 +706,6 @@ export const useGeminiStream = (
isShellFocused,
backgroundTasks,
settings.merged.ui?.compactToolOutput,
isActive,
]);
const lastQueryRef = useRef<PartListUnion | null>(null);
@@ -732,7 +723,6 @@ export const useGeminiStream = (
const prevActiveShellPtyIdRef = useRef<number | null>(null);
useEffect(() => {
if (!isActive) return;
if (
turnCancelledRef.current &&
prevActiveShellPtyIdRef.current !== null &&
@@ -742,10 +732,9 @@ export const useGeminiStream = (
setIsResponding(false);
}
prevActiveShellPtyIdRef.current = activeShellPtyId;
}, [activeShellPtyId, addItem, setIsResponding, isActive]);
}, [activeShellPtyId, addItem, setIsResponding]);
useEffect(() => {
if (!isActive) return;
if (
config.getApprovalMode() === ApprovalMode.YOLO &&
streamingState === StreamingState.Idle
@@ -764,7 +753,7 @@ export const useGeminiStream = (
);
}
}
}, [streamingState, config, history, isActive]);
}, [streamingState, config, history]);
useEffect(() => {
if (!isResponding) {
@@ -822,7 +811,6 @@ export const useGeminiStream = (
const cancelOngoingRequest = useCallback(
(clearBuffer: boolean = true) => {
if (!isActive) return;
// If we are already cancelled, do nothing
if (turnCancelledRef.current) {
if (clearBuffer) {
@@ -934,7 +922,6 @@ export const useGeminiStream = (
toolCalls,
activeShellPtyId,
setIsResponding,
isActive,
],
);
@@ -948,9 +935,8 @@ export const useGeminiStream = (
},
{
isActive:
isActive &&
(streamingState === StreamingState.Responding ||
streamingState === StreamingState.WaitingForConfirmation),
streamingState === StreamingState.Responding ||
streamingState === StreamingState.WaitingForConfirmation,
},
);
@@ -999,16 +985,28 @@ export const useGeminiStream = (
if (postSubmitPrompt) {
localQueryToSendToGemini = postSubmitPrompt;
addItem(
{ type: MessageType.USER, text: trimmedQuery },
userMessageTimestamp,
);
return {
queryToSend: localQueryToSendToGemini,
shouldProceed: true,
};
}
addItem(
{ type: MessageType.USER, text: trimmedQuery },
userMessageTimestamp,
);
return { queryToSend: null, shouldProceed: false };
}
case 'submit_prompt': {
localQueryToSendToGemini = slashCommandResult.content;
addItem(
{ type: MessageType.USER, text: trimmedQuery },
userMessageTimestamp,
);
return {
queryToSend: localQueryToSendToGemini,