From 15586d38462a913bc8867b5c95e5b27d5d00a410 Mon Sep 17 00:00:00 2001 From: Will Li Date: Tue, 8 Jul 2025 17:19:22 -0700 Subject: [PATCH] Fixed auto question timer unmount (#5368) * fixed bug * expanded tests and made better mock * code review: refactor & race conditions * code review, some refactor and reset timer on task switch * rename to onCancelAutoApproval --- webview-ui/src/components/chat/ChatRow.tsx | 5 +- webview-ui/src/components/chat/ChatView.tsx | 82 +++- .../src/components/chat/FollowUpSuggest.tsx | 31 +- .../chat/__tests__/FollowUpSuggest.spec.tsx | 414 ++++++++++++++++++ 4 files changed, 514 insertions(+), 18 deletions(-) create mode 100644 webview-ui/src/components/chat/__tests__/FollowUpSuggest.spec.tsx diff --git a/webview-ui/src/components/chat/ChatRow.tsx b/webview-ui/src/components/chat/ChatRow.tsx index a14535c925..d049c0178a 100644 --- a/webview-ui/src/components/chat/ChatRow.tsx +++ b/webview-ui/src/components/chat/ChatRow.tsx @@ -53,6 +53,7 @@ interface ChatRowProps { onSuggestionClick?: (suggestion: SuggestionItem, event?: React.MouseEvent) => void onBatchFileResponse?: (response: { [key: string]: boolean }) => void onFollowUpUnmount?: () => void + isFollowUpAnswered?: boolean editable?: boolean } @@ -104,6 +105,7 @@ export const ChatRowContent = ({ onSuggestionClick, onFollowUpUnmount, onBatchFileResponse, + isFollowUpAnswered, editable, }: ChatRowContentProps) => { const { t } = useTranslation() @@ -1298,7 +1300,8 @@ export const ChatRowContent = ({ suggestions={followUpData?.suggest} onSuggestionClick={onSuggestionClick} ts={message?.ts} - onUnmount={onFollowUpUnmount} + onCancelAutoApproval={onFollowUpUnmount} + isAnswered={isFollowUpAnswered} /> ) diff --git a/webview-ui/src/components/chat/ChatView.tsx b/webview-ui/src/components/chat/ChatView.tsx index 84bb7b89a2..de2092248d 100644 --- a/webview-ui/src/components/chat/ChatView.tsx +++ b/webview-ui/src/components/chat/ChatView.tsx @@ -169,6 +169,8 @@ const ChatViewComponent: React.ForwardRefRenderFunction(null) + const userRespondedRef = useRef(false) + const [currentFollowUpTs, setCurrentFollowUpTs] = useState(null) const clineAskRef = useRef(clineAsk) useEffect(() => { @@ -415,6 +417,15 @@ const ChatViewComponent: React.ForwardRefRenderFunction { setExpandedRows({}) everVisibleMessagesTsRef.current.clear() // Clear for new task + setCurrentFollowUpTs(null) // Clear follow-up answered state for new task + + // Clear any pending auto-approval timeout from previous task + if (autoApproveTimeoutRef.current) { + clearTimeout(autoApproveTimeoutRef.current) + autoApproveTimeoutRef.current = null + } + // Reset user response flag for new task + userRespondedRef.current = false }, [task?.ts]) useEffect(() => { @@ -486,7 +497,22 @@ const ChatViewComponent: React.ForwardRefRenderFunction { + const lastFollowUpMessage = messagesRef.current.findLast((msg) => msg.ask === "followup") + if (lastFollowUpMessage) { + setCurrentFollowUpTs(lastFollowUpMessage.ts) + } + }, []) + const handleChatReset = useCallback(() => { + // Clear any pending auto-approval timeout + if (autoApproveTimeoutRef.current) { + clearTimeout(autoApproveTimeoutRef.current) + autoApproveTimeoutRef.current = null + } + // Reset user response flag for new message + userRespondedRef.current = false + // Only reset message-specific state, preserving mode. setInputValue("") setSendingDisabled(true) @@ -504,9 +530,16 @@ const ChatViewComponent: React.ForwardRefRenderFunction 0) { + // Mark that user has responded - this prevents any pending auto-approvals + userRespondedRef.current = true + if (messagesRef.current.length === 0) { vscode.postMessage({ type: "newTask", text, images }) } else if (clineAskRef.current) { + if (clineAskRef.current === "followup") { + markFollowUpAsAnswered() + } + // Use clineAskRef.current switch ( clineAskRef.current // Use clineAskRef.current @@ -530,7 +563,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction { + // Mark that user has responded + userRespondedRef.current = true + const trimmedInput = text?.trim() switch (clineAsk) { @@ -599,6 +635,9 @@ const ChatViewComponent: React.ForwardRefRenderFunction { + // Mark that user has responded + userRespondedRef.current = true + const trimmedInput = text?.trim() if (isStreaming) { @@ -1219,6 +1258,16 @@ const ChatViewComponent: React.ForwardRefRenderFunction { + // Mark that user has responded if this is a manual click (not auto-approval) + if (event) { + userRespondedRef.current = true + } + + // Mark the current follow-up question as answered when a suggestion is clicked + if (clineAsk === "followup" && !event?.shiftKey) { + markFollowUpAsAnswered() + } + // Check if we need to switch modes if (suggestion.mode) { // Only switch modes if it's a manual click (event exists) or auto-approval is allowed @@ -1238,7 +1287,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction { @@ -1248,11 +1297,8 @@ const ChatViewComponent: React.ForwardRefRenderFunction { - // Clear the auto-approve timeout to prevent race conditions - if (autoApproveTimeoutRef.current) { - clearTimeout(autoApproveTimeoutRef.current) - autoApproveTimeoutRef.current = null - } + // Mark that user has responded + userRespondedRef.current = true }, []) const itemContent = useCallback( @@ -1291,6 +1337,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction { if (lastMessage?.ask && isAutoApproved(lastMessage)) { // Special handling for follow-up questions @@ -1354,9 +1407,17 @@ const ChatViewComponent: React.ForwardRefRenderFunction 0) { // Wait for the configured timeout before auto-selecting the first suggestion await new Promise((resolve) => { - autoApproveTimeoutRef.current = setTimeout(resolve, followupAutoApproveTimeoutMs) + autoApproveTimeoutRef.current = setTimeout(() => { + autoApproveTimeoutRef.current = null + resolve() + }, followupAutoApproveTimeoutMs) }) + // Check if user responded manually + if (userRespondedRef.current) { + return + } + // Get the first suggestion const firstSuggestion = followUpData.suggest[0] @@ -1366,7 +1427,10 @@ const ChatViewComponent: React.ForwardRefRenderFunction((resolve) => { - autoApproveTimeoutRef.current = setTimeout(resolve, writeDelayMs) + autoApproveTimeoutRef.current = setTimeout(() => { + autoApproveTimeoutRef.current = null + resolve() + }, writeDelayMs) }) } diff --git a/webview-ui/src/components/chat/FollowUpSuggest.tsx b/webview-ui/src/components/chat/FollowUpSuggest.tsx index 5649da744a..1ffe31bbcb 100644 --- a/webview-ui/src/components/chat/FollowUpSuggest.tsx +++ b/webview-ui/src/components/chat/FollowUpSuggest.tsx @@ -14,10 +14,17 @@ interface FollowUpSuggestProps { suggestions?: SuggestionItem[] onSuggestionClick?: (suggestion: SuggestionItem, event?: React.MouseEvent) => void ts: number - onUnmount?: () => void + onCancelAutoApproval?: () => void + isAnswered?: boolean } -export const FollowUpSuggest = ({ suggestions = [], onSuggestionClick, ts = 1, onUnmount }: FollowUpSuggestProps) => { +export const FollowUpSuggest = ({ + suggestions = [], + onSuggestionClick, + ts = 1, + onCancelAutoApproval, + isAnswered = false, +}: FollowUpSuggestProps) => { const { autoApprovalEnabled, alwaysAllowFollowupQuestions, followupAutoApproveTimeoutMs } = useExtensionState() const [countdown, setCountdown] = useState(null) const [suggestionSelected, setSuggestionSelected] = useState(false) @@ -26,7 +33,14 @@ export const FollowUpSuggest = ({ suggestions = [], onSuggestionClick, ts = 1, o // Start countdown timer when auto-approval is enabled for follow-up questions useEffect(() => { // Only start countdown if auto-approval is enabled for follow-up questions and no suggestion has been selected - if (autoApprovalEnabled && alwaysAllowFollowupQuestions && suggestions.length > 0 && !suggestionSelected) { + // Also stop countdown if the question has been answered + if ( + autoApprovalEnabled && + alwaysAllowFollowupQuestions && + suggestions.length > 0 && + !suggestionSelected && + !isAnswered + ) { // Start with the configured timeout in seconds const timeoutMs = typeof followupAutoApproveTimeoutMs === "number" && !isNaN(followupAutoApproveTimeoutMs) @@ -52,7 +66,7 @@ export const FollowUpSuggest = ({ suggestions = [], onSuggestionClick, ts = 1, o clearInterval(intervalId) // Notify parent component that this component is unmounting // so it can clear any related timeouts - onUnmount?.() + onCancelAutoApproval?.() } } else { setCountdown(null) @@ -63,7 +77,8 @@ export const FollowUpSuggest = ({ suggestions = [], onSuggestionClick, ts = 1, o suggestions, followupAutoApproveTimeoutMs, suggestionSelected, - onUnmount, + onCancelAutoApproval, + isAnswered, ]) const handleSuggestionClick = useCallback( (suggestion: SuggestionItem, event: React.MouseEvent) => { @@ -72,14 +87,14 @@ export const FollowUpSuggest = ({ suggestions = [], onSuggestionClick, ts = 1, o setSuggestionSelected(true) // Also notify parent component to cancel auto-approval timeout // This prevents race conditions between visual countdown and actual timeout - onUnmount?.() + onCancelAutoApproval?.() } // Pass the suggestion object to the parent component // The parent component will handle mode switching if needed onSuggestionClick?.(suggestion, event) }, - [onSuggestionClick, onUnmount], + [onSuggestionClick, onCancelAutoApproval], ) // Don't render if there are no suggestions or no click handler. @@ -100,7 +115,7 @@ export const FollowUpSuggest = ({ suggestions = [], onSuggestionClick, ts = 1, o onClick={(event) => handleSuggestionClick(suggestion, event)} aria-label={suggestion.answer}> {suggestion.answer} - {isFirstSuggestion && countdown !== null && !suggestionSelected && ( + {isFirstSuggestion && countdown !== null && !suggestionSelected && !isAnswered && ( diff --git a/webview-ui/src/components/chat/__tests__/FollowUpSuggest.spec.tsx b/webview-ui/src/components/chat/__tests__/FollowUpSuggest.spec.tsx new file mode 100644 index 0000000000..aa3f84fd8c --- /dev/null +++ b/webview-ui/src/components/chat/__tests__/FollowUpSuggest.spec.tsx @@ -0,0 +1,414 @@ +import React, { createContext, useContext } from "react" +import { render, screen, act } from "@testing-library/react" + +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest" +import { FollowUpSuggest } from "../FollowUpSuggest" +import { TooltipProvider } from "@radix-ui/react-tooltip" + +// Mock the translation hook +vi.mock("@src/i18n/TranslationContext", () => ({ + TranslationProvider: ({ children }: { children: React.ReactNode }) => children, + useAppTranslation: () => ({ + t: (key: string, options?: any) => { + if (key === "chat:followUpSuggest.countdownDisplay" && options?.count !== undefined) { + return `${options.count}s` + } + if (key === "chat:followUpSuggest.autoSelectCountdown" && options?.count !== undefined) { + return `Auto-selecting in ${options.count} seconds` + } + if (key === "chat:followUpSuggest.copyToInput") { + return "Copy to input" + } + return key + }, + }), +})) + +// Test-specific extension state context that only provides the values needed by FollowUpSuggest +interface TestExtensionState { + autoApprovalEnabled: boolean + alwaysAllowFollowupQuestions: boolean + followupAutoApproveTimeoutMs: number +} + +const TestExtensionStateContext = createContext(undefined) + +// Mock the useExtensionState hook to use our test context +vi.mock("@src/context/ExtensionStateContext", () => ({ + useExtensionState: () => { + const context = useContext(TestExtensionStateContext) + if (!context) { + throw new Error("useExtensionState must be used within TestExtensionStateProvider") + } + return context + }, +})) + +// Test provider that only provides the specific values needed by FollowUpSuggest +const TestExtensionStateProvider: React.FC<{ + children: React.ReactNode + value: TestExtensionState +}> = ({ children, value }) => { + return {children} +} + +// Helper function to render component with test providers +const renderWithTestProviders = (component: React.ReactElement, extensionState: TestExtensionState) => { + return render( + + {component} + , + ) +} + +describe("FollowUpSuggest", () => { + const mockSuggestions = [{ answer: "First suggestion" }, { answer: "Second suggestion" }] + + const mockOnSuggestionClick = vi.fn() + const mockOnCancelAutoApproval = vi.fn() + + // Default test state with auto-approval enabled + const defaultTestState: TestExtensionState = { + autoApprovalEnabled: true, + alwaysAllowFollowupQuestions: true, + followupAutoApproveTimeoutMs: 3000, // 3 seconds for testing + } + + beforeEach(() => { + vi.clearAllMocks() + vi.useFakeTimers() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it("should display countdown timer when auto-approval is enabled", () => { + renderWithTestProviders( + , + defaultTestState, + ) + + // Should show initial countdown (3 seconds) + expect(screen.getByText(/3s/)).toBeInTheDocument() + }) + + it("should not display countdown timer when isAnswered is true", () => { + renderWithTestProviders( + , + defaultTestState, + ) + + // Should not show countdown + expect(screen.queryByText(/\d+s/)).not.toBeInTheDocument() + }) + + it("should clear interval and call onCancelAutoApproval when component unmounts", () => { + const { unmount } = renderWithTestProviders( + , + defaultTestState, + ) + + // Unmount the component + unmount() + + // onCancelAutoApproval should have been called + expect(mockOnCancelAutoApproval).toHaveBeenCalled() + }) + + it("should not show countdown when auto-approval is disabled", () => { + const testState: TestExtensionState = { + ...defaultTestState, + autoApprovalEnabled: false, + } + + renderWithTestProviders( + , + testState, + ) + + // Should not show countdown + expect(screen.queryByText(/\d+s/)).not.toBeInTheDocument() + }) + + it("should not show countdown when alwaysAllowFollowupQuestions is false", () => { + const testState: TestExtensionState = { + ...defaultTestState, + alwaysAllowFollowupQuestions: false, + } + + renderWithTestProviders( + , + testState, + ) + + // Should not show countdown + expect(screen.queryByText(/\d+s/)).not.toBeInTheDocument() + }) + + it("should use custom timeout value from extension state", () => { + const testState: TestExtensionState = { + ...defaultTestState, + followupAutoApproveTimeoutMs: 5000, // 5 seconds + } + + renderWithTestProviders( + , + testState, + ) + + // Should show initial countdown (5 seconds) + expect(screen.getByText(/5s/)).toBeInTheDocument() + }) + + it("should render suggestions without countdown when both auto-approval settings are disabled", () => { + const testState: TestExtensionState = { + autoApprovalEnabled: false, + alwaysAllowFollowupQuestions: false, + followupAutoApproveTimeoutMs: 3000, + } + + renderWithTestProviders( + , + testState, + ) + + // Should render suggestions + expect(screen.getByText("First suggestion")).toBeInTheDocument() + expect(screen.getByText("Second suggestion")).toBeInTheDocument() + + // Should not show countdown + expect(screen.queryByText(/\d+s/)).not.toBeInTheDocument() + }) + + it("should not render when no suggestions are provided", () => { + const { container } = renderWithTestProviders( + , + defaultTestState, + ) + + // Component should not render anything + expect(container.firstChild).toBeNull() + }) + + it("should not render when onSuggestionClick is not provided", () => { + const { container } = renderWithTestProviders( + , + defaultTestState, + ) + + // Component should not render anything + expect(container.firstChild).toBeNull() + }) + + it("should stop countdown when user manually responds (isAnswered becomes true)", () => { + const { rerender } = renderWithTestProviders( + , + defaultTestState, + ) + + // Initially should show countdown + expect(screen.getByText(/3s/)).toBeInTheDocument() + + // Simulate user manually responding by setting isAnswered to true + rerender( + + + + + , + ) + + // Countdown should no longer be visible immediately after isAnswered becomes true + expect(screen.queryByText(/\d+s/)).not.toBeInTheDocument() + + // Advance timer to ensure countdown doesn't restart or continue + vi.advanceTimersByTime(5000) + + // onSuggestionClick should not have been called (auto-selection stopped) + expect(mockOnSuggestionClick).not.toHaveBeenCalled() + + // Countdown should still not be visible + expect(screen.queryByText(/\d+s/)).not.toBeInTheDocument() + + // Verify onCancelAutoApproval was called when the countdown was stopped + expect(mockOnCancelAutoApproval).toHaveBeenCalled() + }) + + it("should handle race condition when timeout fires but user has already responded", () => { + // This test simulates the scenario where: + // 1. Auto-approval countdown starts + // 2. User manually responds (isAnswered becomes true) + // 3. The timeout still fires (because it was already scheduled) + // 4. The auto-selection should NOT happen because user already responded + + const { rerender } = renderWithTestProviders( + , + defaultTestState, + ) + + // Initially should show countdown + expect(screen.getByText(/3s/)).toBeInTheDocument() + + // Advance timer to just before timeout completes (2.5 seconds) + vi.advanceTimersByTime(2500) + + // User manually responds before timeout completes + rerender( + + + + + , + ) + + // Countdown should be hidden immediately + expect(screen.queryByText(/\d+s/)).not.toBeInTheDocument() + + // Now advance timer past the original timeout duration + vi.advanceTimersByTime(1000) // Total: 3.5 seconds + + // onSuggestionClick should NOT have been called + // This verifies the fix for the race condition + expect(mockOnSuggestionClick).not.toHaveBeenCalled() + }) + + it("should update countdown display as time progresses", async () => { + renderWithTestProviders( + , + defaultTestState, + ) + + // Initially should show 3s + expect(screen.getByText(/3s/)).toBeInTheDocument() + + // Advance timer by 1 second and wait for React to update + await act(async () => { + vi.advanceTimersByTime(1000) + }) + + // Check countdown updated to 2s + expect(screen.getByText(/2s/)).toBeInTheDocument() + + // Advance timer by another second + await act(async () => { + vi.advanceTimersByTime(1000) + }) + + // Check countdown updated to 1s + expect(screen.getByText(/1s/)).toBeInTheDocument() + + // Advance timer to completion - countdown should disappear + await act(async () => { + vi.advanceTimersByTime(1000) + }) + + // Countdown should no longer be visible after reaching 0 + expect(screen.queryByText(/\d+s/)).not.toBeInTheDocument() + + // The component itself doesn't trigger auto-selection, that's handled by ChatView + expect(mockOnSuggestionClick).not.toHaveBeenCalled() + }) + + it("should handle component unmounting during countdown", () => { + const { unmount } = renderWithTestProviders( + , + defaultTestState, + ) + + // Initially should show countdown + expect(screen.getByText(/3s/)).toBeInTheDocument() + + // Advance timer partially + vi.advanceTimersByTime(1500) + + // Unmount component before countdown completes + unmount() + + // onCancelAutoApproval should have been called + expect(mockOnCancelAutoApproval).toHaveBeenCalled() + + // Advance timer past the original timeout + vi.advanceTimersByTime(2000) + + // onSuggestionClick should NOT have been called (component doesn't auto-select) + expect(mockOnSuggestionClick).not.toHaveBeenCalled() + }) +})