code review

Author: liwilliam2021

packages/types/src/followup.ts

@@ -0,0 +1,41 @@
+import { z } from "zod"
+
+/**
+ * Interface for follow-up data structure used in follow-up questions
+ * This represents the data structure for follow-up questions that the LLM can ask
+ * to gather more information needed to complete a task.
+ */
+export interface FollowUpData {
+	/** The question being asked by the LLM */
+	question?: string
+	/** Array of suggested answers that the user can select */
+	suggest?: Array<string | SuggestionItem>
+}
+
+/**
+ * Interface for a suggestion item with optional mode switching
+ */
+export interface SuggestionItem {
+	/** The text of the suggestion */
+	answer: string
+	/** Optional mode to switch to when selecting this suggestion */
+	mode?: string
+}
+
+/**
+ * Zod schema for SuggestionItem
+ */
+export const suggestionItemSchema = z.object({
+	answer: z.string(),
+	mode: z.string().optional(),
+})
+
+/**
+ * Zod schema for FollowUpData
+ */
+export const followUpDataSchema = z.object({
+	question: z.string().optional(),
+	suggest: z.array(z.union([z.string(), suggestionItemSchema])).optional(),
+})
+
+export type FollowUpDataType = z.infer<typeof followUpDataSchema>

packages/types/src/index.ts

@@ -4,6 +4,7 @@ export * from "./api.js"
 export * from "./codebase-index.js"
 export * from "./cloud.js"
 export * from "./experiment.js"
+export * from "./followup.js"
 export * from "./global-settings.js"
 export * from "./history.js"
 export * from "./ipc.js"

src/core/webview/ClineProvider.ts

@@ -1524,7 +1524,7 @@ export class ClineProvider
 			cloudApiUrl: getRooCodeApiUrl(),
 			hasOpenedModeSelector: this.getGlobalState("hasOpenedModeSelector") ?? false,
 			alwaysAllowFollowupQuestions: alwaysAllowFollowupQuestions ?? false,
-			followupAutoApproveTimeoutMs: followupAutoApproveTimeoutMs ?? 10000,
+			followupAutoApproveTimeoutMs: followupAutoApproveTimeoutMs ?? 60000,
 		}
 	}
 
@@ -1606,7 +1606,7 @@ export class ClineProvider
 			alwaysAllowModeSwitch: stateValues.alwaysAllowModeSwitch ?? false,
 			alwaysAllowSubtasks: stateValues.alwaysAllowSubtasks ?? false,
 			alwaysAllowFollowupQuestions: stateValues.alwaysAllowFollowupQuestions ?? false,
-			followupAutoApproveTimeoutMs: stateValues.followupAutoApproveTimeoutMs ?? 10000,
+			followupAutoApproveTimeoutMs: stateValues.followupAutoApproveTimeoutMs ?? 60000,
 			allowedMaxRequests: stateValues.allowedMaxRequests,
 			autoCondenseContext: stateValues.autoCondenseContext ?? true,
 			autoCondenseContextPercent: stateValues.autoCondenseContextPercent ?? 100,

webview-ui/src/components/chat/ChatRow.tsx

@@ -10,6 +10,7 @@ import type { ClineMessage } from "@roo-code/types"
 import { ClineApiReqInfo, ClineAskUseMcpServer, ClineSayTool } from "@roo/ExtensionMessage"
 import { COMMAND_OUTPUT_STRING } from "@roo/combineCommandSequences"
 import { safeJsonParse } from "@roo/safeJsonParse"
+import { FollowUpData, SuggestionItem } from "@roo-code/types"
 
 import { useCopyToClipboard } from "@src/utils/clipboard"
 import { useExtensionState } from "@src/context/ExtensionStateContext"
@@ -50,6 +51,7 @@ interface ChatRowProps {
 	onHeightChange: (isTaller: boolean) => void
 	onSuggestionClick?: (answer: string, event?: React.MouseEvent) => void
 	onBatchFileResponse?: (response: { [key: string]: boolean }) => void
+	onFollowUpUnmount?: () => void
 }
 
 // eslint-disable-next-line @typescript-eslint/no-empty-object-type
@@ -98,6 +100,7 @@ export const ChatRowContent = ({
 	isStreaming,
 	onToggleExpand,
 	onSuggestionClick,
+	onFollowUpUnmount,
 	onBatchFileResponse,
 }: ChatRowContentProps) => {
 	const { t } = useTranslation()
@@ -279,7 +282,7 @@ export const ChatRowContent = ({
 
 	const followUpData = useMemo(() => {
 		if (message.type === "ask" && message.ask === "followup" && !message.partial) {
-			return safeJsonParse<any>(message.text)
+			return safeJsonParse<FollowUpData>(message.text)
 		}
 		return null
 	}, [message.type, message.ask, message.partial, message.text])
@@ -1215,6 +1218,7 @@ export const ChatRowContent = ({
 								suggestions={followUpData?.suggest}
 								onSuggestionClick={onSuggestionClick}
 								ts={message?.ts}
+								onUnmount={onFollowUpUnmount}
 							/>
 						</>
 					)

webview-ui/src/components/chat/ChatView.tsx

@@ -15,6 +15,7 @@ import type { ClineAsk, ClineMessage } from "@roo-code/types"
 import { ClineSayBrowserAction, ClineSayTool, ExtensionMessage } from "@roo/ExtensionMessage"
 import { McpServer, McpTool } from "@roo/mcp"
 import { findLast } from "@roo/array"
+import { FollowUpData } from "@roo-code/types"
 import { combineApiRequests } from "@roo/combineApiRequests"
 import { combineCommandSequences } from "@roo/combineCommandSequences"
 import { getApiMetrics } from "@roo/getApiMetrics"
@@ -1215,6 +1216,15 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
 		vscode.postMessage({ type: "askResponse", askResponse: "objectResponse", text: JSON.stringify(response) })
 	}, [])
 
+	// Handler for when FollowUpSuggest component unmounts
+	const handleFollowUpUnmount = useCallback(() => {
+		// Clear the auto-approve timeout to prevent race conditions
+		if (autoApproveTimeoutRef.current) {
+			clearTimeout(autoApproveTimeoutRef.current)
+			autoApproveTimeoutRef.current = null
+		}
+	}, [])
+
 	const itemContent = useCallback(
 		(index: number, messageOrGroup: ClineMessage | ClineMessage[]) => {
 			// browser session group
@@ -1250,6 +1260,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
 					isStreaming={isStreaming}
 					onSuggestionClick={handleSuggestionClickInRow} // This was already stabilized
 					onBatchFileResponse={handleBatchFileResponse}
+					onFollowUpUnmount={handleFollowUpUnmount}
 				/>
 			)
 		},
@@ -1262,6 +1273,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
 			isStreaming,
 			handleSuggestionClickInRow,
 			handleBatchFileResponse,
+			handleFollowUpUnmount,
 		],
 	)
 
@@ -1279,7 +1291,15 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
 			if (lastMessage?.ask && isAutoApproved(lastMessage)) {
 				// Special handling for follow-up questions
 				if (lastMessage.ask === "followup") {
-					const followUpData = JSON.parse(lastMessage.text || "{}")
+					// Handle invalid JSON
+					let followUpData: FollowUpData = {}
+					try {
+						followUpData = JSON.parse(lastMessage.text || "{}") as FollowUpData
+					} catch (error) {
+						console.error("Failed to parse follow-up data:", error)
+						return
+					}
+
 					if (followUpData && followUpData.suggest && followUpData.suggest.length > 0) {
 						// Wait for the configured timeout before auto-selecting the first suggestion
 						await new Promise<void>((resolve) => {

webview-ui/src/components/chat/FollowUpSuggest.tsx

@@ -6,19 +6,19 @@ import { vscode } from "@/utils/vscode"
 
 import { useAppTranslation } from "@src/i18n/TranslationContext"
 import { useExtensionState } from "@src/context/ExtensionStateContext"
+import { SuggestionItem } from "@roo-code/types"
 
-interface SuggestionItem {
-	answer: string
-	mode?: string
-}
+const DEFAULT_FOLLOWUP_TIMEOUT_MS = 60000
+const COUNTDOWN_INTERVAL_MS = 1000
 
 interface FollowUpSuggestProps {
 	suggestions?: (string | SuggestionItem)[]
 	onSuggestionClick?: (answer: string, event?: React.MouseEvent) => void
 	ts: number
+	onUnmount?: () => void
 }
 
-export const FollowUpSuggest = ({ suggestions = [], onSuggestionClick, ts = 1 }: FollowUpSuggestProps) => {
+export const FollowUpSuggest = ({ suggestions = [], onSuggestionClick, ts = 1, onUnmount }: FollowUpSuggestProps) => {
 	const { autoApprovalEnabled, alwaysAllowFollowupQuestions, followupAutoApproveTimeoutMs } = useExtensionState()
 	const [countdown, setCountdown] = useState<number | null>(null)
 	const [suggestionSelected, setSuggestionSelected] = useState(false)
@@ -32,7 +32,7 @@ export const FollowUpSuggest = ({ suggestions = [], onSuggestionClick, ts = 1 }:
 			const timeoutMs =
 				typeof followupAutoApproveTimeoutMs === "number" && !isNaN(followupAutoApproveTimeoutMs)
 					? followupAutoApproveTimeoutMs
-					: 10000
+					: DEFAULT_FOLLOWUP_TIMEOUT_MS
 
 			// Convert milliseconds to seconds for the countdown
 			setCountdown(Math.floor(timeoutMs / 1000))
@@ -46,10 +46,15 @@ export const FollowUpSuggest = ({ suggestions = [], onSuggestionClick, ts = 1 }:
 					}
 					return prevCountdown - 1
 				})
-			}, 1000)
+			}, COUNTDOWN_INTERVAL_MS)
 
-			// Clean up interval on unmount
-			return () => clearInterval(intervalId)
+			// Clean up interval on unmount and notify parent component
+			return () => {
+				clearInterval(intervalId)
+				// Notify parent component that this component is unmounting
+				// so it can clear any related timeouts
+				onUnmount?.()
+			}
 		} else {
 			setCountdown(null)
 		}
@@ -76,11 +81,14 @@ export const FollowUpSuggest = ({ suggestions = [], onSuggestionClick, ts = 1 }:
 			// Mark a suggestion as selected if it's not a shift-click (which just copies to input)
 			if (!event.shiftKey) {
 				setSuggestionSelected(true)
+				// Also notify parent component to cancel auto-approval timeout
+				// This prevents race conditions between visual countdown and actual timeout
+				onUnmount?.()
 			}
 
 			onSuggestionClick?.(suggestionText, event)
 		},
-		[onSuggestionClick],
+		[onSuggestionClick, onUnmount],
 	)
 
 	// Don't render if there are no suggestions or no click handler.

webview-ui/src/components/settings/AutoApproveSettings.tsx

@@ -63,7 +63,7 @@ export const AutoApproveSettings = ({
 	alwaysAllowSubtasks,
 	alwaysAllowExecute,
 	alwaysAllowFollowupQuestions,
-	followupAutoApproveTimeoutMs = 10000,
+	followupAutoApproveTimeoutMs = 60000,
 	allowedCommands,
 	setCachedStateField,
 	...props
@@ -219,7 +219,7 @@ export const AutoApproveSettings = ({
 							<div className="flex items-center gap-2">
 								<Slider
 									min={1000}
-									max={30000}
+									max={300000}
 									step={1000}
 									value={[followupAutoApproveTimeoutMs]}
 									onValueChange={([value]) =>

webview-ui/src/components/settings/SettingsView.tsx

@@ -604,7 +604,7 @@ const SettingsView = forwardRef<SettingsViewRef, SettingsViewProps>(({ onDone, t
 							alwaysAllowSubtasks={alwaysAllowSubtasks}
 							alwaysAllowExecute={alwaysAllowExecute}
 							alwaysAllowFollowupQuestions={alwaysAllowFollowupQuestions}
-							followupAutoApproveTimeoutMs={followupAutoApproveTimeoutMs || 10000}
+							followupAutoApproveTimeoutMs={followupAutoApproveTimeoutMs}
 							allowedCommands={allowedCommands}
 							setCachedStateField={setCachedStateField}
 						/>