From aeea95b313df135664360863c1e0847569eef678 Mon Sep 17 00:00:00 2001 From: Will Li Date: Wed, 2 Jul 2025 21:29:47 -0700 Subject: [PATCH 1/5] fixed bug --- webview-ui/src/components/chat/ChatRow.tsx | 3 + webview-ui/src/components/chat/ChatView.tsx | 35 ++- .../src/components/chat/FollowUpSuggest.tsx | 21 +- .../chat/__tests__/FollowUpSuggest.spec.tsx | 200 ++++++++++++++++++ 4 files changed, 255 insertions(+), 4 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 f61eb8c5e0..7845b8efad 100644 --- a/webview-ui/src/components/chat/ChatRow.tsx +++ b/webview-ui/src/components/chat/ChatRow.tsx @@ -52,6 +52,7 @@ interface ChatRowProps { onSuggestionClick?: (suggestion: SuggestionItem, event?: React.MouseEvent) => void onBatchFileResponse?: (response: { [key: string]: boolean }) => void onFollowUpUnmount?: () => void + isFollowUpAnswered?: boolean } // eslint-disable-next-line @typescript-eslint/no-empty-object-type @@ -102,6 +103,7 @@ export const ChatRowContent = ({ onSuggestionClick, onFollowUpUnmount, onBatchFileResponse, + isFollowUpAnswered, }: ChatRowContentProps) => { const { t } = useTranslation() const { mcpServers, alwaysAllowMcp, currentCheckpoint } = useExtensionState() @@ -1219,6 +1221,7 @@ export const ChatRowContent = ({ onSuggestionClick={onSuggestionClick} ts={message?.ts} onUnmount={onFollowUpUnmount} + isAnswered={isFollowUpAnswered} /> ) diff --git a/webview-ui/src/components/chat/ChatView.tsx b/webview-ui/src/components/chat/ChatView.tsx index 874f0de3c2..041fb343bf 100644 --- a/webview-ui/src/components/chat/ChatView.tsx +++ b/webview-ui/src/components/chat/ChatView.tsx @@ -163,6 +163,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction(null) + const [followUpAnswered, setFollowUpAnswered] = useState>(new Set()) const clineAskRef = useRef(clineAsk) useEffect(() => { @@ -415,6 +416,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction { setExpandedRows({}) everVisibleMessagesTsRef.current.clear() // Clear for new task + setFollowUpAnswered(new Set()) // Clear follow-up answered state for new task }, [task?.ts]) useEffect(() => { @@ -487,6 +489,12 @@ const ChatViewComponent: React.ForwardRefRenderFunction { + // Clear any pending auto-approval timeout to prevent race conditions + if (autoApproveTimeoutRef.current) { + clearTimeout(autoApproveTimeoutRef.current) + autoApproveTimeoutRef.current = null + } + // Only reset message-specific state, preserving mode. setInputValue("") setSendingDisabled(true) @@ -507,6 +515,21 @@ const ChatViewComponent: React.ForwardRefRenderFunction msg.ask === "followup") + if (lastFollowUpMessage) { + setFollowUpAnswered((prev) => new Set(prev).add(lastFollowUpMessage.ts)) + } + } + // Use clineAskRef.current switch ( clineAskRef.current // Use clineAskRef.current @@ -1214,6 +1237,14 @@ const ChatViewComponent: React.ForwardRefRenderFunction { + // Mark the current follow-up question as answered when a suggestion is clicked + if (clineAsk === "followup" && !event?.shiftKey) { + const lastFollowUpMessage = messagesRef.current.findLast((msg) => msg.ask === "followup") + if (lastFollowUpMessage) { + setFollowUpAnswered((prev) => new Set(prev).add(lastFollowUpMessage.ts)) + } + } + // 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 @@ -1233,7 +1264,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction { @@ -1286,6 +1317,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction ) }, @@ -1299,6 +1331,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction void ts: number onUnmount?: () => void + isAnswered?: boolean } -export const FollowUpSuggest = ({ suggestions = [], onSuggestionClick, ts = 1, onUnmount }: FollowUpSuggestProps) => { +export const FollowUpSuggest = ({ + suggestions = [], + onSuggestionClick, + ts = 1, + onUnmount, + 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) @@ -64,6 +78,7 @@ export const FollowUpSuggest = ({ suggestions = [], onSuggestionClick, ts = 1, o followupAutoApproveTimeoutMs, suggestionSelected, onUnmount, + isAnswered, ]) const handleSuggestionClick = useCallback( (suggestion: SuggestionItem, event: React.MouseEvent) => { @@ -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..d3e08caab0 --- /dev/null +++ b/webview-ui/src/components/chat/__tests__/FollowUpSuggest.spec.tsx @@ -0,0 +1,200 @@ +import { render, screen } from "@testing-library/react" +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest" +import { FollowUpSuggest } from "../FollowUpSuggest" +import { ExtensionStateContext, ExtensionStateContextType } from "@src/context/ExtensionStateContext" +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 + }, + }), +})) + +// Mock the extension state +const createMockExtensionState = (overrides?: Partial): ExtensionStateContextType => + ({ + version: "1.0.0", + clineMessages: [], + taskHistory: [], + shouldShowAnnouncement: false, + allowedCommands: [], + soundEnabled: false, + soundVolume: 0.5, + ttsEnabled: false, + ttsSpeed: 1.0, + diffEnabled: false, + enableCheckpoints: true, + fuzzyMatchThreshold: 1.0, + language: "en", + writeDelayMs: 1000, + browserViewportSize: "900x600", + screenshotQuality: 75, + terminalOutputLineLimit: 500, + terminalShellIntegrationTimeout: 4000, + mcpEnabled: true, + enableMcpServerCreation: false, + alwaysApproveResubmit: false, + requestDelaySeconds: 5, + currentApiConfigName: "default", + listApiConfigMeta: [], + mode: "code", + customModePrompts: {}, + customSupportPrompts: {}, + experiments: {}, + enhancementApiConfigId: "", + condensingApiConfigId: "", + customCondensingPrompt: "", + hasOpenedModeSelector: false, + autoApprovalEnabled: true, + alwaysAllowFollowupQuestions: true, + followupAutoApproveTimeoutMs: 3000, // 3 seconds for testing + customModes: [], + maxOpenTabsContext: 20, + maxWorkspaceFiles: 200, + cwd: "", + browserToolEnabled: true, + telemetrySetting: "unset", + showRooIgnoredFiles: true, + renderContext: "sidebar", + maxReadFileLine: -1, + pinnedApiConfigs: {}, + didHydrateState: true, + showWelcome: false, + theme: {}, + mcpServers: [], + filePaths: [], + openedTabs: [], + organizationAllowList: { type: "all" }, + cloudIsAuthenticated: false, + sharingEnabled: false, + mdmCompliant: true, + autoCondenseContext: false, + autoCondenseContextPercent: 50, + setHasOpenedModeSelector: vi.fn(), + setAlwaysAllowFollowupQuestions: vi.fn(), + setFollowupAutoApproveTimeoutMs: vi.fn(), + setCondensingApiConfigId: vi.fn(), + setCustomCondensingPrompt: vi.fn(), + setPinnedApiConfigs: vi.fn(), + togglePinnedApiConfig: vi.fn(), + setTerminalCompressProgressBar: vi.fn(), + setHistoryPreviewCollapsed: vi.fn(), + setAutoCondenseContext: vi.fn(), + setAutoCondenseContextPercent: vi.fn(), + ...overrides, + }) as ExtensionStateContextType + +const renderWithProviders = (component: React.ReactElement, stateOverrides?: Partial) => { + const mockState = createMockExtensionState(stateOverrides) + + return render( + + {component} + , + ) +} + +describe("FollowUpSuggest", () => { + const mockSuggestions = [{ answer: "First suggestion" }, { answer: "Second suggestion" }] + + const mockOnSuggestionClick = vi.fn() + const mockOnUnmount = vi.fn() + + beforeEach(() => { + vi.clearAllMocks() + vi.useFakeTimers() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it("should display countdown timer when auto-approval is enabled", () => { + renderWithProviders( + , + ) + + // Should show initial countdown (3 seconds) + expect(screen.getByText(/3s/)).toBeInTheDocument() + }) + + it("should not display countdown timer when isAnswered is true", () => { + renderWithProviders( + , + ) + + // Should not show countdown + expect(screen.queryByText(/\d+s/)).not.toBeInTheDocument() + }) + + it("should clear interval and call onUnmount when component unmounts", () => { + const { unmount } = renderWithProviders( + , + ) + + // Unmount the component + unmount() + + // onUnmount should have been called + expect(mockOnUnmount).toHaveBeenCalled() + }) + + it("should not show countdown when auto-approval is disabled", () => { + renderWithProviders( + , + { autoApprovalEnabled: false }, + ) + + // Should not show countdown + expect(screen.queryByText(/\d+s/)).not.toBeInTheDocument() + }) + + it("should not show countdown when alwaysAllowFollowupQuestions is false", () => { + renderWithProviders( + , + { alwaysAllowFollowupQuestions: false }, + ) + + // Should not show countdown + expect(screen.queryByText(/\d+s/)).not.toBeInTheDocument() + }) +}) From 78f92b7c543374dfa099dc1851da951152261f8c Mon Sep 17 00:00:00 2001 From: Will Li Date: Thu, 3 Jul 2025 13:42:23 -0700 Subject: [PATCH 2/5] expanded tests and made better mock --- .../chat/__tests__/FollowUpSuggest.spec.tsx | 261 ++++++++++++------ 1 file changed, 175 insertions(+), 86 deletions(-) diff --git a/webview-ui/src/components/chat/__tests__/FollowUpSuggest.spec.tsx b/webview-ui/src/components/chat/__tests__/FollowUpSuggest.spec.tsx index d3e08caab0..b628c657bb 100644 --- a/webview-ui/src/components/chat/__tests__/FollowUpSuggest.spec.tsx +++ b/webview-ui/src/components/chat/__tests__/FollowUpSuggest.spec.tsx @@ -1,7 +1,7 @@ +import React, { createContext, useContext } from "react" import { render, screen } from "@testing-library/react" import { describe, it, expect, vi, beforeEach, afterEach } from "vitest" import { FollowUpSuggest } from "../FollowUpSuggest" -import { ExtensionStateContext, ExtensionStateContextType } from "@src/context/ExtensionStateContext" import { TooltipProvider } from "@radix-ui/react-tooltip" // Mock the translation hook @@ -23,87 +23,40 @@ vi.mock("@src/i18n/TranslationContext", () => ({ }), })) -// Mock the extension state -const createMockExtensionState = (overrides?: Partial): ExtensionStateContextType => - ({ - version: "1.0.0", - clineMessages: [], - taskHistory: [], - shouldShowAnnouncement: false, - allowedCommands: [], - soundEnabled: false, - soundVolume: 0.5, - ttsEnabled: false, - ttsSpeed: 1.0, - diffEnabled: false, - enableCheckpoints: true, - fuzzyMatchThreshold: 1.0, - language: "en", - writeDelayMs: 1000, - browserViewportSize: "900x600", - screenshotQuality: 75, - terminalOutputLineLimit: 500, - terminalShellIntegrationTimeout: 4000, - mcpEnabled: true, - enableMcpServerCreation: false, - alwaysApproveResubmit: false, - requestDelaySeconds: 5, - currentApiConfigName: "default", - listApiConfigMeta: [], - mode: "code", - customModePrompts: {}, - customSupportPrompts: {}, - experiments: {}, - enhancementApiConfigId: "", - condensingApiConfigId: "", - customCondensingPrompt: "", - hasOpenedModeSelector: false, - autoApprovalEnabled: true, - alwaysAllowFollowupQuestions: true, - followupAutoApproveTimeoutMs: 3000, // 3 seconds for testing - customModes: [], - maxOpenTabsContext: 20, - maxWorkspaceFiles: 200, - cwd: "", - browserToolEnabled: true, - telemetrySetting: "unset", - showRooIgnoredFiles: true, - renderContext: "sidebar", - maxReadFileLine: -1, - pinnedApiConfigs: {}, - didHydrateState: true, - showWelcome: false, - theme: {}, - mcpServers: [], - filePaths: [], - openedTabs: [], - organizationAllowList: { type: "all" }, - cloudIsAuthenticated: false, - sharingEnabled: false, - mdmCompliant: true, - autoCondenseContext: false, - autoCondenseContextPercent: 50, - setHasOpenedModeSelector: vi.fn(), - setAlwaysAllowFollowupQuestions: vi.fn(), - setFollowupAutoApproveTimeoutMs: vi.fn(), - setCondensingApiConfigId: vi.fn(), - setCustomCondensingPrompt: vi.fn(), - setPinnedApiConfigs: vi.fn(), - togglePinnedApiConfig: vi.fn(), - setTerminalCompressProgressBar: vi.fn(), - setHistoryPreviewCollapsed: vi.fn(), - setAutoCondenseContext: vi.fn(), - setAutoCondenseContextPercent: vi.fn(), - ...overrides, - }) as ExtensionStateContextType - -const renderWithProviders = (component: React.ReactElement, stateOverrides?: Partial) => { - const mockState = createMockExtensionState(stateOverrides) +// 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} - , + , ) } @@ -113,6 +66,13 @@ describe("FollowUpSuggest", () => { const mockOnSuggestionClick = vi.fn() const mockOnUnmount = 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() @@ -123,13 +83,14 @@ describe("FollowUpSuggest", () => { }) it("should display countdown timer when auto-approval is enabled", () => { - renderWithProviders( + renderWithTestProviders( , + defaultTestState, ) // Should show initial countdown (3 seconds) @@ -137,7 +98,7 @@ describe("FollowUpSuggest", () => { }) it("should not display countdown timer when isAnswered is true", () => { - renderWithProviders( + renderWithTestProviders( { onUnmount={mockOnUnmount} isAnswered={true} />, + defaultTestState, ) // Should not show countdown @@ -152,13 +114,14 @@ describe("FollowUpSuggest", () => { }) it("should clear interval and call onUnmount when component unmounts", () => { - const { unmount } = renderWithProviders( + const { unmount } = renderWithTestProviders( , + defaultTestState, ) // Unmount the component @@ -169,14 +132,19 @@ describe("FollowUpSuggest", () => { }) it("should not show countdown when auto-approval is disabled", () => { - renderWithProviders( + const testState: TestExtensionState = { + ...defaultTestState, + autoApprovalEnabled: false, + } + + renderWithTestProviders( , - { autoApprovalEnabled: false }, + testState, ) // Should not show countdown @@ -184,17 +152,138 @@ describe("FollowUpSuggest", () => { }) it("should not show countdown when alwaysAllowFollowupQuestions is false", () => { - renderWithProviders( + 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( , - { alwaysAllowFollowupQuestions: false }, + 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 onUnmount was called when the countdown was stopped + expect(mockOnUnmount).toHaveBeenCalled() + }) }) From 04b7b35b89c4db357a3be9c6cd904adc64a82ea7 Mon Sep 17 00:00:00 2001 From: Will Li Date: Thu, 3 Jul 2025 14:15:50 -0700 Subject: [PATCH 3/5] code review: refactor & race conditions --- webview-ui/src/components/chat/ChatView.tsx | 53 +++++++++++++-------- 1 file changed, 32 insertions(+), 21 deletions(-) diff --git a/webview-ui/src/components/chat/ChatView.tsx b/webview-ui/src/components/chat/ChatView.tsx index 041fb343bf..be08a55288 100644 --- a/webview-ui/src/components/chat/ChatView.tsx +++ b/webview-ui/src/components/chat/ChatView.tsx @@ -163,7 +163,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction(null) - const [followUpAnswered, setFollowUpAnswered] = useState>(new Set()) + const [currentFollowUpTs, setCurrentFollowUpTs] = useState(null) const clineAskRef = useRef(clineAsk) useEffect(() => { @@ -416,7 +416,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction { setExpandedRows({}) everVisibleMessagesTsRef.current.clear() // Clear for new task - setFollowUpAnswered(new Set()) // Clear follow-up answered state for new task + setCurrentFollowUpTs(null) // Clear follow-up answered state for new task }, [task?.ts]) useEffect(() => { @@ -488,6 +488,13 @@ 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 to prevent race conditions if (autoApproveTimeoutRef.current) { @@ -515,19 +522,15 @@ const ChatViewComponent: React.ForwardRefRenderFunction msg.ask === "followup") - if (lastFollowUpMessage) { - setFollowUpAnswered((prev) => new Set(prev).add(lastFollowUpMessage.ts)) - } + if (clineAskRef.current === "followup") { + markFollowUpAsAnswered() } // Use clineAskRef.current @@ -553,7 +556,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction { + // Clear auto-approval timeout to prevent race conditions when suggestion is clicked + if (autoApproveTimeoutRef.current) { + clearTimeout(autoApproveTimeoutRef.current) + autoApproveTimeoutRef.current = null + } + // Mark the current follow-up question as answered when a suggestion is clicked if (clineAsk === "followup" && !event?.shiftKey) { - const lastFollowUpMessage = messagesRef.current.findLast((msg) => msg.ask === "followup") - if (lastFollowUpMessage) { - setFollowUpAnswered((prev) => new Set(prev).add(lastFollowUpMessage.ts)) - } + markFollowUpAsAnswered() } // Check if we need to switch modes @@ -1264,7 +1270,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction { @@ -1317,7 +1323,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction ) }, @@ -1331,7 +1337,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction Date: Sun, 6 Jul 2025 23:51:24 -0700 Subject: [PATCH 4/5] code review, some refactor and reset timer on task switch --- webview-ui/src/components/chat/ChatView.tsx | 62 ++++++--- .../chat/__tests__/FollowUpSuggest.spec.tsx | 127 +++++++++++++++++- 2 files changed, 167 insertions(+), 22 deletions(-) diff --git a/webview-ui/src/components/chat/ChatView.tsx b/webview-ui/src/components/chat/ChatView.tsx index be08a55288..65dd0b393f 100644 --- a/webview-ui/src/components/chat/ChatView.tsx +++ b/webview-ui/src/components/chat/ChatView.tsx @@ -163,6 +163,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction(null) + const userRespondedRef = useRef(false) const [currentFollowUpTs, setCurrentFollowUpTs] = useState(null) const clineAskRef = useRef(clineAsk) @@ -417,6 +418,14 @@ const ChatViewComponent: React.ForwardRefRenderFunction { @@ -496,11 +505,13 @@ const ChatViewComponent: React.ForwardRefRenderFunction { - // Clear any pending auto-approval timeout to prevent race conditions + // 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("") @@ -519,16 +530,12 @@ 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) { - // Clear auto-approval timeout FIRST to prevent race conditions - // This ensures the countdown timer is properly dismounted when users submit custom responses - if (autoApproveTimeoutRef.current) { - clearTimeout(autoApproveTimeoutRef.current) - autoApproveTimeoutRef.current = null - } - if (clineAskRef.current === "followup") { markFollowUpAsAnswered() } @@ -581,6 +588,9 @@ const ChatViewComponent: React.ForwardRefRenderFunction { + // Mark that user has responded + userRespondedRef.current = true + const trimmedInput = text?.trim() switch (clineAsk) { @@ -625,6 +635,9 @@ const ChatViewComponent: React.ForwardRefRenderFunction { + // Mark that user has responded + userRespondedRef.current = true + const trimmedInput = text?.trim() if (isStreaming) { @@ -1240,10 +1253,9 @@ const ChatViewComponent: React.ForwardRefRenderFunction { - // Clear auto-approval timeout to prevent race conditions when suggestion is clicked - if (autoApproveTimeoutRef.current) { - clearTimeout(autoApproveTimeoutRef.current) - autoApproveTimeoutRef.current = null + // 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 @@ -1280,11 +1292,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( @@ -1351,6 +1360,11 @@ const ChatViewComponent: React.ForwardRefRenderFunction { if (lastMessage?.ask && isAutoApproved(lastMessage)) { // Special handling for follow-up questions @@ -1367,11 +1381,14 @@ 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 timeout was cleared (user responded manually) - if (!autoApproveTimeoutRef.current) { + // Check if user responded manually + if (userRespondedRef.current) { return } @@ -1384,7 +1401,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/__tests__/FollowUpSuggest.spec.tsx b/webview-ui/src/components/chat/__tests__/FollowUpSuggest.spec.tsx index b628c657bb..764b648318 100644 --- a/webview-ui/src/components/chat/__tests__/FollowUpSuggest.spec.tsx +++ b/webview-ui/src/components/chat/__tests__/FollowUpSuggest.spec.tsx @@ -1,5 +1,6 @@ import React, { createContext, useContext } from "react" -import { render, screen } from "@testing-library/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" @@ -286,4 +287,128 @@ describe("FollowUpSuggest", () => { // Verify onUnmount was called when the countdown was stopped expect(mockOnUnmount).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() + + // onUnmount should have been called + expect(mockOnUnmount).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() + }) }) From 2d49c169cf3c02c238ef5e1ad4571b4801cba4f3 Mon Sep 17 00:00:00 2001 From: Will Li Date: Mon, 7 Jul 2025 14:57:26 -0700 Subject: [PATCH 5/5] rename to onCancelAutoApproval --- webview-ui/src/components/chat/ChatRow.tsx | 2 +- .../src/components/chat/FollowUpSuggest.tsx | 12 ++--- .../chat/__tests__/FollowUpSuggest.spec.tsx | 46 +++++++++---------- 3 files changed, 30 insertions(+), 30 deletions(-) diff --git a/webview-ui/src/components/chat/ChatRow.tsx b/webview-ui/src/components/chat/ChatRow.tsx index 7b773b9de5..f687320a65 100644 --- a/webview-ui/src/components/chat/ChatRow.tsx +++ b/webview-ui/src/components/chat/ChatRow.tsx @@ -1240,7 +1240,7 @@ 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/FollowUpSuggest.tsx b/webview-ui/src/components/chat/FollowUpSuggest.tsx index 9e7788a8ce..1ffe31bbcb 100644 --- a/webview-ui/src/components/chat/FollowUpSuggest.tsx +++ b/webview-ui/src/components/chat/FollowUpSuggest.tsx @@ -14,7 +14,7 @@ interface FollowUpSuggestProps { suggestions?: SuggestionItem[] onSuggestionClick?: (suggestion: SuggestionItem, event?: React.MouseEvent) => void ts: number - onUnmount?: () => void + onCancelAutoApproval?: () => void isAnswered?: boolean } @@ -22,7 +22,7 @@ export const FollowUpSuggest = ({ suggestions = [], onSuggestionClick, ts = 1, - onUnmount, + onCancelAutoApproval, isAnswered = false, }: FollowUpSuggestProps) => { const { autoApprovalEnabled, alwaysAllowFollowupQuestions, followupAutoApproveTimeoutMs } = useExtensionState() @@ -66,7 +66,7 @@ export const FollowUpSuggest = ({ clearInterval(intervalId) // Notify parent component that this component is unmounting // so it can clear any related timeouts - onUnmount?.() + onCancelAutoApproval?.() } } else { setCountdown(null) @@ -77,7 +77,7 @@ export const FollowUpSuggest = ({ suggestions, followupAutoApproveTimeoutMs, suggestionSelected, - onUnmount, + onCancelAutoApproval, isAnswered, ]) const handleSuggestionClick = useCallback( @@ -87,14 +87,14 @@ export const FollowUpSuggest = ({ 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. diff --git a/webview-ui/src/components/chat/__tests__/FollowUpSuggest.spec.tsx b/webview-ui/src/components/chat/__tests__/FollowUpSuggest.spec.tsx index 764b648318..aa3f84fd8c 100644 --- a/webview-ui/src/components/chat/__tests__/FollowUpSuggest.spec.tsx +++ b/webview-ui/src/components/chat/__tests__/FollowUpSuggest.spec.tsx @@ -65,7 +65,7 @@ describe("FollowUpSuggest", () => { const mockSuggestions = [{ answer: "First suggestion" }, { answer: "Second suggestion" }] const mockOnSuggestionClick = vi.fn() - const mockOnUnmount = vi.fn() + const mockOnCancelAutoApproval = vi.fn() // Default test state with auto-approval enabled const defaultTestState: TestExtensionState = { @@ -89,7 +89,7 @@ describe("FollowUpSuggest", () => { suggestions={mockSuggestions} onSuggestionClick={mockOnSuggestionClick} ts={123} - onUnmount={mockOnUnmount} + onCancelAutoApproval={mockOnCancelAutoApproval} />, defaultTestState, ) @@ -104,7 +104,7 @@ describe("FollowUpSuggest", () => { suggestions={mockSuggestions} onSuggestionClick={mockOnSuggestionClick} ts={123} - onUnmount={mockOnUnmount} + onCancelAutoApproval={mockOnCancelAutoApproval} isAnswered={true} />, defaultTestState, @@ -114,13 +114,13 @@ describe("FollowUpSuggest", () => { expect(screen.queryByText(/\d+s/)).not.toBeInTheDocument() }) - it("should clear interval and call onUnmount when component unmounts", () => { + it("should clear interval and call onCancelAutoApproval when component unmounts", () => { const { unmount } = renderWithTestProviders( , defaultTestState, ) @@ -128,8 +128,8 @@ describe("FollowUpSuggest", () => { // Unmount the component unmount() - // onUnmount should have been called - expect(mockOnUnmount).toHaveBeenCalled() + // onCancelAutoApproval should have been called + expect(mockOnCancelAutoApproval).toHaveBeenCalled() }) it("should not show countdown when auto-approval is disabled", () => { @@ -143,7 +143,7 @@ describe("FollowUpSuggest", () => { suggestions={mockSuggestions} onSuggestionClick={mockOnSuggestionClick} ts={123} - onUnmount={mockOnUnmount} + onCancelAutoApproval={mockOnCancelAutoApproval} />, testState, ) @@ -163,7 +163,7 @@ describe("FollowUpSuggest", () => { suggestions={mockSuggestions} onSuggestionClick={mockOnSuggestionClick} ts={123} - onUnmount={mockOnUnmount} + onCancelAutoApproval={mockOnCancelAutoApproval} />, testState, ) @@ -183,7 +183,7 @@ describe("FollowUpSuggest", () => { suggestions={mockSuggestions} onSuggestionClick={mockOnSuggestionClick} ts={123} - onUnmount={mockOnUnmount} + onCancelAutoApproval={mockOnCancelAutoApproval} />, testState, ) @@ -204,7 +204,7 @@ describe("FollowUpSuggest", () => { suggestions={mockSuggestions} onSuggestionClick={mockOnSuggestionClick} ts={123} - onUnmount={mockOnUnmount} + onCancelAutoApproval={mockOnCancelAutoApproval} />, testState, ) @@ -223,7 +223,7 @@ describe("FollowUpSuggest", () => { suggestions={[]} onSuggestionClick={mockOnSuggestionClick} ts={123} - onUnmount={mockOnUnmount} + onCancelAutoApproval={mockOnCancelAutoApproval} />, defaultTestState, ) @@ -234,7 +234,7 @@ describe("FollowUpSuggest", () => { it("should not render when onSuggestionClick is not provided", () => { const { container } = renderWithTestProviders( - , + , defaultTestState, ) @@ -248,7 +248,7 @@ describe("FollowUpSuggest", () => { suggestions={mockSuggestions} onSuggestionClick={mockOnSuggestionClick} ts={123} - onUnmount={mockOnUnmount} + onCancelAutoApproval={mockOnCancelAutoApproval} isAnswered={false} />, defaultTestState, @@ -265,7 +265,7 @@ describe("FollowUpSuggest", () => { suggestions={mockSuggestions} onSuggestionClick={mockOnSuggestionClick} ts={123} - onUnmount={mockOnUnmount} + onCancelAutoApproval={mockOnCancelAutoApproval} isAnswered={true} /> @@ -284,8 +284,8 @@ describe("FollowUpSuggest", () => { // Countdown should still not be visible expect(screen.queryByText(/\d+s/)).not.toBeInTheDocument() - // Verify onUnmount was called when the countdown was stopped - expect(mockOnUnmount).toHaveBeenCalled() + // 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", () => { @@ -300,7 +300,7 @@ describe("FollowUpSuggest", () => { suggestions={mockSuggestions} onSuggestionClick={mockOnSuggestionClick} ts={123} - onUnmount={mockOnUnmount} + onCancelAutoApproval={mockOnCancelAutoApproval} isAnswered={false} />, defaultTestState, @@ -320,7 +320,7 @@ describe("FollowUpSuggest", () => { suggestions={mockSuggestions} onSuggestionClick={mockOnSuggestionClick} ts={123} - onUnmount={mockOnUnmount} + onCancelAutoApproval={mockOnCancelAutoApproval} isAnswered={true} /> @@ -344,7 +344,7 @@ describe("FollowUpSuggest", () => { suggestions={mockSuggestions} onSuggestionClick={mockOnSuggestionClick} ts={123} - onUnmount={mockOnUnmount} + onCancelAutoApproval={mockOnCancelAutoApproval} isAnswered={false} />, defaultTestState, @@ -387,7 +387,7 @@ describe("FollowUpSuggest", () => { suggestions={mockSuggestions} onSuggestionClick={mockOnSuggestionClick} ts={123} - onUnmount={mockOnUnmount} + onCancelAutoApproval={mockOnCancelAutoApproval} isAnswered={false} />, defaultTestState, @@ -402,8 +402,8 @@ describe("FollowUpSuggest", () => { // Unmount component before countdown completes unmount() - // onUnmount should have been called - expect(mockOnUnmount).toHaveBeenCalled() + // onCancelAutoApproval should have been called + expect(mockOnCancelAutoApproval).toHaveBeenCalled() // Advance timer past the original timeout vi.advanceTimersByTime(2000)