Skip to content

Commit e269841

Browse files
liwilliam2021liwilliam2021
committed
code review, some refactor and reset timer on task switch
1 parent 04b7b35 commit e269841

File tree

2 files changed

+167
-22
lines changed

2 files changed

+167
-22
lines changed

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

Lines changed: 41 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
163163
}),
164164
)
165165
const autoApproveTimeoutRef = useRef<NodeJS.Timeout | null>(null)
166+
const userRespondedRef = useRef<boolean>(false)
166167
const [currentFollowUpTs, setCurrentFollowUpTs] = useState<number | null>(null)
167168

168169
const clineAskRef = useRef(clineAsk)
@@ -417,6 +418,14 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
417418
setExpandedRows({})
418419
everVisibleMessagesTsRef.current.clear() // Clear for new task
419420
setCurrentFollowUpTs(null) // Clear follow-up answered state for new task
421+
422+
// Clear any pending auto-approval timeout from previous task
423+
if (autoApproveTimeoutRef.current) {
424+
clearTimeout(autoApproveTimeoutRef.current)
425+
autoApproveTimeoutRef.current = null
426+
}
427+
// Reset user response flag for new task
428+
userRespondedRef.current = false
420429
}, [task?.ts])
421430

422431
useEffect(() => {
@@ -496,11 +505,13 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
496505
}, [])
497506

498507
const handleChatReset = useCallback(() => {
499-
// Clear any pending auto-approval timeout to prevent race conditions
508+
// Clear any pending auto-approval timeout
500509
if (autoApproveTimeoutRef.current) {
501510
clearTimeout(autoApproveTimeoutRef.current)
502511
autoApproveTimeoutRef.current = null
503512
}
513+
// Reset user response flag for new message
514+
userRespondedRef.current = false
504515

505516
// Only reset message-specific state, preserving mode.
506517
setInputValue("")
@@ -519,16 +530,12 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
519530
text = text.trim()
520531

521532
if (text || images.length > 0) {
533+
// Mark that user has responded - this prevents any pending auto-approvals
534+
userRespondedRef.current = true
535+
522536
if (messagesRef.current.length === 0) {
523537
vscode.postMessage({ type: "newTask", text, images })
524538
} else if (clineAskRef.current) {
525-
// Clear auto-approval timeout FIRST to prevent race conditions
526-
// This ensures the countdown timer is properly dismounted when users submit custom responses
527-
if (autoApproveTimeoutRef.current) {
528-
clearTimeout(autoApproveTimeoutRef.current)
529-
autoApproveTimeoutRef.current = null
530-
}
531-
532539
if (clineAskRef.current === "followup") {
533540
markFollowUpAsAnswered()
534541
}
@@ -581,6 +588,9 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
581588
// extension.
582589
const handlePrimaryButtonClick = useCallback(
583590
(text?: string, images?: string[]) => {
591+
// Mark that user has responded
592+
userRespondedRef.current = true
593+
584594
const trimmedInput = text?.trim()
585595

586596
switch (clineAsk) {
@@ -625,6 +635,9 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
625635

626636
const handleSecondaryButtonClick = useCallback(
627637
(text?: string, images?: string[]) => {
638+
// Mark that user has responded
639+
userRespondedRef.current = true
640+
628641
const trimmedInput = text?.trim()
629642

630643
if (isStreaming) {
@@ -1240,10 +1253,9 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
12401253

12411254
const handleSuggestionClickInRow = useCallback(
12421255
(suggestion: SuggestionItem, event?: React.MouseEvent) => {
1243-
// Clear auto-approval timeout to prevent race conditions when suggestion is clicked
1244-
if (autoApproveTimeoutRef.current) {
1245-
clearTimeout(autoApproveTimeoutRef.current)
1246-
autoApproveTimeoutRef.current = null
1256+
// Mark that user has responded if this is a manual click (not auto-approval)
1257+
if (event) {
1258+
userRespondedRef.current = true
12471259
}
12481260

12491261
// Mark the current follow-up question as answered when a suggestion is clicked
@@ -1280,11 +1292,8 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
12801292

12811293
// Handler for when FollowUpSuggest component unmounts
12821294
const handleFollowUpUnmount = useCallback(() => {
1283-
// Clear the auto-approve timeout to prevent race conditions
1284-
if (autoApproveTimeoutRef.current) {
1285-
clearTimeout(autoApproveTimeoutRef.current)
1286-
autoApproveTimeoutRef.current = null
1287-
}
1295+
// Mark that user has responded
1296+
userRespondedRef.current = true
12881297
}, [])
12891298

12901299
const itemContent = useCallback(
@@ -1351,6 +1360,11 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
13511360
return
13521361
}
13531362

1363+
// Exit early if user has already responded
1364+
if (userRespondedRef.current) {
1365+
return
1366+
}
1367+
13541368
const autoApprove = async () => {
13551369
if (lastMessage?.ask && isAutoApproved(lastMessage)) {
13561370
// Special handling for follow-up questions
@@ -1367,11 +1381,14 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
13671381
if (followUpData && followUpData.suggest && followUpData.suggest.length > 0) {
13681382
// Wait for the configured timeout before auto-selecting the first suggestion
13691383
await new Promise<void>((resolve) => {
1370-
autoApproveTimeoutRef.current = setTimeout(resolve, followupAutoApproveTimeoutMs)
1384+
autoApproveTimeoutRef.current = setTimeout(() => {
1385+
autoApproveTimeoutRef.current = null
1386+
resolve()
1387+
}, followupAutoApproveTimeoutMs)
13711388
})
13721389

1373-
// Check if timeout was cleared (user responded manually)
1374-
if (!autoApproveTimeoutRef.current) {
1390+
// Check if user responded manually
1391+
if (userRespondedRef.current) {
13751392
return
13761393
}
13771394

@@ -1384,7 +1401,10 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
13841401
}
13851402
} else if (lastMessage.ask === "tool" && isWriteToolAction(lastMessage)) {
13861403
await new Promise<void>((resolve) => {
1387-
autoApproveTimeoutRef.current = setTimeout(resolve, writeDelayMs)
1404+
autoApproveTimeoutRef.current = setTimeout(() => {
1405+
autoApproveTimeoutRef.current = null
1406+
resolve()
1407+
}, writeDelayMs)
13881408
})
13891409
}
13901410

‎webview-ui/src/components/chat/__tests__/FollowUpSuggest.spec.tsx‎

Lines changed: 126 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import React, { createContext, useContext } from "react"
2-
import { render, screen } from "@testing-library/react"
2+
import { render, screen, act } from "@testing-library/react"
3+
34
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"
45
import { FollowUpSuggest } from "../FollowUpSuggest"
56
import { TooltipProvider } from "@radix-ui/react-tooltip"
@@ -286,4 +287,128 @@ describe("FollowUpSuggest", () => {
286287
// Verify onUnmount was called when the countdown was stopped
287288
expect(mockOnUnmount).toHaveBeenCalled()
288289
})
290+
291+
it("should handle race condition when timeout fires but user has already responded", () => {
292+
// This test simulates the scenario where:
293+
// 1. Auto-approval countdown starts
294+
// 2. User manually responds (isAnswered becomes true)
295+
// 3. The timeout still fires (because it was already scheduled)
296+
// 4. The auto-selection should NOT happen because user already responded
297+
298+
const { rerender } = renderWithTestProviders(
299+
<FollowUpSuggest
300+
suggestions={mockSuggestions}
301+
onSuggestionClick={mockOnSuggestionClick}
302+
ts={123}
303+
onUnmount={mockOnUnmount}
304+
isAnswered={false}
305+
/>,
306+
defaultTestState,
307+
)
308+
309+
// Initially should show countdown
310+
expect(screen.getByText(/3s/)).toBeInTheDocument()
311+
312+
// Advance timer to just before timeout completes (2.5 seconds)
313+
vi.advanceTimersByTime(2500)
314+
315+
// User manually responds before timeout completes
316+
rerender(
317+
<TestExtensionStateProvider value={defaultTestState}>
318+
<TooltipProvider>
319+
<FollowUpSuggest
320+
suggestions={mockSuggestions}
321+
onSuggestionClick={mockOnSuggestionClick}
322+
ts={123}
323+
onUnmount={mockOnUnmount}
324+
isAnswered={true}
325+
/>
326+
</TooltipProvider>
327+
</TestExtensionStateProvider>,
328+
)
329+
330+
// Countdown should be hidden immediately
331+
expect(screen.queryByText(/\d+s/)).not.toBeInTheDocument()
332+
333+
// Now advance timer past the original timeout duration
334+
vi.advanceTimersByTime(1000) // Total: 3.5 seconds
335+
336+
// onSuggestionClick should NOT have been called
337+
// This verifies the fix for the race condition
338+
expect(mockOnSuggestionClick).not.toHaveBeenCalled()
339+
})
340+
341+
it("should update countdown display as time progresses", async () => {
342+
renderWithTestProviders(
343+
<FollowUpSuggest
344+
suggestions={mockSuggestions}
345+
onSuggestionClick={mockOnSuggestionClick}
346+
ts={123}
347+
onUnmount={mockOnUnmount}
348+
isAnswered={false}
349+
/>,
350+
defaultTestState,
351+
)
352+
353+
// Initially should show 3s
354+
expect(screen.getByText(/3s/)).toBeInTheDocument()
355+
356+
// Advance timer by 1 second and wait for React to update
357+
await act(async () => {
358+
vi.advanceTimersByTime(1000)
359+
})
360+
361+
// Check countdown updated to 2s
362+
expect(screen.getByText(/2s/)).toBeInTheDocument()
363+
364+
// Advance timer by another second
365+
await act(async () => {
366+
vi.advanceTimersByTime(1000)
367+
})
368+
369+
// Check countdown updated to 1s
370+
expect(screen.getByText(/1s/)).toBeInTheDocument()
371+
372+
// Advance timer to completion - countdown should disappear
373+
await act(async () => {
374+
vi.advanceTimersByTime(1000)
375+
})
376+
377+
// Countdown should no longer be visible after reaching 0
378+
expect(screen.queryByText(/\d+s/)).not.toBeInTheDocument()
379+
380+
// The component itself doesn't trigger auto-selection, that's handled by ChatView
381+
expect(mockOnSuggestionClick).not.toHaveBeenCalled()
382+
})
383+
384+
it("should handle component unmounting during countdown", () => {
385+
const { unmount } = renderWithTestProviders(
386+
<FollowUpSuggest
387+
suggestions={mockSuggestions}
388+
onSuggestionClick={mockOnSuggestionClick}
389+
ts={123}
390+
onUnmount={mockOnUnmount}
391+
isAnswered={false}
392+
/>,
393+
defaultTestState,
394+
)
395+
396+
// Initially should show countdown
397+
expect(screen.getByText(/3s/)).toBeInTheDocument()
398+
399+
// Advance timer partially
400+
vi.advanceTimersByTime(1500)
401+
402+
// Unmount component before countdown completes
403+
unmount()
404+
405+
// onUnmount should have been called
406+
expect(mockOnUnmount).toHaveBeenCalled()
407+
408+
// Advance timer past the original timeout
409+
vi.advanceTimersByTime(2000)
410+
411+
// onSuggestionClick should NOT have been called (component doesn't auto-select)
412+
expect(mockOnSuggestionClick).not.toHaveBeenCalled()
413+
})
289414
})

0 commit comments

Comments
 (0)