Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions webview-ui/eslint-suppressions.json
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{}
5 changes: 3 additions & 2 deletions webview-ui/src/components/chat/FollowUpSuggest.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,15 @@ export const FollowUpSuggest = ({
// 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
// Also stop countdown if the question has been answered or auto-approval is paused (user is typing)
// Also stop countdown if the question has been answered or auto-approval is paused (user is typing) or timer is disabled (set to 0)
if (
autoApprovalEnabled &&
alwaysAllowFollowupQuestions &&
suggestions.length > 0 &&
!suggestionSelected &&
!isAnswered &&
!isFollowUpAutoApprovalPaused
!isFollowUpAutoApprovalPaused &&
(followupAutoApproveTimeoutMs ?? DEFAULT_FOLLOWUP_TIMEOUT_MS) > 0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing clamps a negative followupAutoApproveTimeoutMs before it reaches this guard, so a > 0 vs !== 0 mutation would be indistinguishable on the values the suite tests (0, 3000, 5000). Worth adding a negative-value case?

) {
// Start with the configured timeout in seconds
const timeoutMs =
Expand Down
136 changes: 134 additions & 2 deletions webview-ui/src/components/chat/__tests__/FollowUpSuggest.spec.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React, { createContext, useContext } from "react"
import { render, screen, act } from "@testing-library/react"
import { render, screen, act, fireEvent } from "@testing-library/react"
import { TooltipProvider } from "@radix-ui/react-tooltip"

import { FollowUpSuggest } from "../FollowUpSuggest"
Expand Down Expand Up @@ -28,7 +28,7 @@ vi.mock("@src/i18n/TranslationContext", () => ({
interface TestExtensionState {
autoApprovalEnabled: boolean
alwaysAllowFollowupQuestions: boolean
followupAutoApproveTimeoutMs: number
followupAutoApproveTimeoutMs?: number
}

const TestExtensionStateContext = createContext<TestExtensionState | undefined>(undefined)
Expand Down Expand Up @@ -74,6 +74,13 @@ describe("FollowUpSuggest", () => {
followupAutoApproveTimeoutMs: 3000, // 3 seconds for testing
}

// Test state with timeout disabled (0)
const disabledTimeoutState: TestExtensionState = {
autoApprovalEnabled: true,
alwaysAllowFollowupQuestions: true,
followupAutoApproveTimeoutMs: 0, // Disabled
}

beforeEach(() => {
vi.clearAllMocks()
vi.useFakeTimers()
Expand Down Expand Up @@ -218,6 +225,41 @@ describe("FollowUpSuggest", () => {
expect(screen.queryByText(/\d+s/)).not.toBeInTheDocument()
})

// Should not show countdown when timeout is disabled (set to 0)
it("should not show countdown when timeout is disabled (set to 0)", () => {
renderWithTestProviders(
<FollowUpSuggest
suggestions={mockSuggestions}
onSuggestionClick={mockOnSuggestionClick}
ts={1}
onCancelAutoApproval={mockOnCancelAutoApproval}
/>,
disabledTimeoutState,
)

// Should not show countdown when timeout is disabled
expect(screen.queryByText(/\d+s/)).not.toBeInTheDocument()
})

it("should not show countdown when timeout is negative", () => {
const negativeTimeoutState: TestExtensionState = {
...defaultTestState,
followupAutoApproveTimeoutMs: -1000,
}

renderWithTestProviders(
<FollowUpSuggest
suggestions={mockSuggestions}
onSuggestionClick={mockOnSuggestionClick}
ts={1}
onCancelAutoApproval={mockOnCancelAutoApproval}
/>,
negativeTimeoutState,
)

expect(screen.queryByText(/\d+s/)).not.toBeInTheDocument()
})

it("should not render when no suggestions are provided", () => {
const { container } = renderWithTestProviders(
<FollowUpSuggest
Expand Down Expand Up @@ -707,4 +749,94 @@ describe("FollowUpSuggest", () => {
expect(mockOnCancelAutoApproval).toHaveBeenCalled()
})
})

describe("suggestion interactions", () => {
it("cancels countdown and forwards click when user clicks a suggestion", () => {
renderWithTestProviders(
<FollowUpSuggest
suggestions={mockSuggestions}
onSuggestionClick={mockOnSuggestionClick}
ts={123}
onCancelAutoApproval={mockOnCancelAutoApproval}
/>,
defaultTestState,
)

fireEvent.click(screen.getByText("First suggestion"))

expect(mockOnSuggestionClick).toHaveBeenCalledWith(
expect.objectContaining({ answer: "First suggestion" }),
expect.objectContaining({ shiftKey: false }),
)
expect(mockOnCancelAutoApproval).toHaveBeenCalled()
expect(screen.queryByText(/Selecting in \d+s/)).not.toBeInTheDocument()
})

it("keeps countdown when shift-clicking a suggestion", () => {
renderWithTestProviders(
<FollowUpSuggest
suggestions={mockSuggestions}
onSuggestionClick={mockOnSuggestionClick}
ts={123}
onCancelAutoApproval={mockOnCancelAutoApproval}
/>,
defaultTestState,
)

mockOnCancelAutoApproval.mockClear()
fireEvent.click(screen.getByText("First suggestion"), { shiftKey: true })

expect(mockOnSuggestionClick).toHaveBeenCalledWith(
expect.objectContaining({ answer: "First suggestion" }),
expect.objectContaining({ shiftKey: true }),
)
expect(mockOnCancelAutoApproval).not.toHaveBeenCalled()
expect(screen.getByText(/Selecting in 3s/)).toBeInTheDocument()
})

it("copies suggestion into input when the copy affordance is clicked", () => {
const { container } = renderWithTestProviders(
<FollowUpSuggest
suggestions={mockSuggestions}
onSuggestionClick={mockOnSuggestionClick}
ts={123}
onCancelAutoApproval={mockOnCancelAutoApproval}
/>,
defaultTestState,
)

const copyAffordance = container.querySelector(
".absolute.cursor-pointer.top-1\\.5.right-1\\.5",
) as HTMLElement

expect(copyAffordance).toBeTruthy()
fireEvent.click(copyAffordance)

expect(mockOnSuggestionClick).toHaveBeenCalledWith(
expect.objectContaining({ answer: "First suggestion" }),
expect.objectContaining({ shiftKey: true }),
)
expect(mockOnCancelAutoApproval).toHaveBeenCalled()
expect(screen.queryByText(/Selecting in \d+s/)).not.toBeInTheDocument()
})

it("uses default timeout when extension state timeout is undefined", () => {
const stateWithUndefinedTimeout = {
...defaultTestState,
followupAutoApproveTimeoutMs: undefined,
}

renderWithTestProviders(
<FollowUpSuggest
suggestions={mockSuggestions}
onSuggestionClick={mockOnSuggestionClick}
ts={123}
onCancelAutoApproval={mockOnCancelAutoApproval}
/>,
stateWithUndefinedTimeout,
)

expect(screen.getByText(/Selecting in 60s/)).toBeInTheDocument()
})
})
})
8 changes: 6 additions & 2 deletions webview-ui/src/components/settings/AutoApproveSettings.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ export const AutoApproveSettings = ({
label={t("settings:autoApprove.followupQuestions.timeoutLabel")}>
<div className="flex items-center gap-2">
<Slider
min={1000}
min={0}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing AutoApproveSettings.spec.tsx never sets alwaysAllowFollowupQuestions: true, so this whole block — including the new min=0 and the 0 → "Disabled" branch — never mounts under test. Worth adding a 0/"Disabled" and a non-zero value case?

max={300000}
step={1000}
value={[followupAutoApproveTimeoutMs]}
Expand All @@ -263,7 +263,11 @@ export const AutoApproveSettings = ({
}
data-testid="followup-timeout-slider"
/>
<span className="w-20">{followupAutoApproveTimeoutMs / 1000}s</span>
<span className="w-20">

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a user-visible settings state, consider adding a Playwright CT snapshot of the auto-approve timeout row showing the "Disabled" label. The settings screen already has visual tests to follow (ModelInfoView.visual.tsx, OpenAICompatible.visual.tsx), and the authoring pattern is in webview-ui/AGENTS.md under Visual Tests.

{followupAutoApproveTimeoutMs === 0
? t("settings:autoApprove.followupQuestions.timeoutDisabled")
: `${followupAutoApproveTimeoutMs / 1000}s`}
</span>
Comment thread
coderabbitai[bot] marked this conversation as resolved.
</div>
<div className="text-vscode-descriptionForeground text-sm mt-1">
{t("settings:autoApprove.followupQuestions.timeoutLabel")}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,24 @@ vi.mock("@/hooks/useAutoApprovalState", () => ({
useAutoApprovalState: () => ({ effectiveAutoApprovalEnabled: false, hasEnabledOptions: false }),
}))

vi.mock("@/components/ui", async (importOriginal) => {
const actual = await importOriginal<typeof import("@/components/ui")>()

return {
...actual,
Button: ({ children, ...props }: any) => <button {...props}>{children}</button>,
Input: (props: any) => <input {...props} />,
Slider: ({ value, onValueChange, ...props }: any) => (
<input
type="range"
value={value?.[0] ?? 0}
onChange={(event) => onValueChange?.([Number((event.target as HTMLInputElement).value)])}
{...props}
/>
),
}
})

const renderSettings = (overrides = {}) => {
const setCachedStateField = vi.fn()
const props = {
Expand Down Expand Up @@ -161,4 +179,71 @@ describe("AutoApproveSettings - Save/Discard contract", () => {
expect(screen.getByTestId("allowed-commands-heading")).toBeInTheDocument()
expect(screen.getByTestId("denied-commands-heading")).toBeInTheDocument()
})

it("renders disabled timeout label when follow-up auto-approve timeout is 0", () => {
const { setCachedStateField } = renderSettings({
alwaysAllowFollowupQuestions: true,
followupAutoApproveTimeoutMs: 0,
})

const slider = screen.getByTestId("followup-timeout-slider") as HTMLInputElement
expect(slider).toBeInTheDocument()
expect(slider.value).toBe("0")
expect(screen.getByText("settings:autoApprove.followupQuestions.timeoutDisabled")).toBeInTheDocument()

fireEvent.change(slider, { target: { value: "4000" } })

expect(setCachedStateField).toHaveBeenCalledWith("followupAutoApproveTimeoutMs", 4000)
expectNoImmediateUpdateSettings()
})

it("renders timeout in seconds when follow-up auto-approve timeout is non-zero", () => {
const { setCachedStateField } = renderSettings({
alwaysAllowFollowupQuestions: true,
followupAutoApproveTimeoutMs: 5000,
})

const slider = screen.getByTestId("followup-timeout-slider") as HTMLInputElement
expect(slider).toBeInTheDocument()
expect(slider.value).toBe("5000")
expect(screen.getByText("5s")).toBeInTheDocument()

fireEvent.change(slider, { target: { value: "0" } })

expect(setCachedStateField).toHaveBeenCalledWith("followupAutoApproveTimeoutMs", 0)
expectNoImmediateUpdateSettings()
})

it("uses the default timeout value when timeout is unset and follow-up auto-approve is enabled", () => {
renderSettings({ alwaysAllowFollowupQuestions: true })

const slider = screen.getByTestId("followup-timeout-slider") as HTMLInputElement
expect(slider.value).toBe("60000")
expect(screen.getByText("60s")).toBeInTheDocument()
})

it("does not render the follow-up timeout controls when follow-up auto-approve is disabled or unset", () => {
const { rerender } = render(
<AutoApproveSettings
alwaysAllowExecute
allowedCommands={[]}
deniedCommands={[]}
alwaysAllowFollowupQuestions={false}
setCachedStateField={vi.fn()}
/>,
)

expect(screen.queryByTestId("followup-timeout-slider")).not.toBeInTheDocument()

rerender(
<AutoApproveSettings
alwaysAllowExecute
allowedCommands={[]}
deniedCommands={[]}
setCachedStateField={vi.fn()}
/>,
)

expect(screen.queryByTestId("followup-timeout-slider")).not.toBeInTheDocument()
})
Comment thread
coderabbitai[bot] marked this conversation as resolved.
})
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/* v8 ignore file -- Playwright component fixture is covered by the visual test. */
import React from "react"

import { TranslationContext } from "@/i18n/TranslationContext"
import i18next from "@/i18n/setup"
import { ExtensionStateContextProvider } from "@/context/ExtensionStateContext"
import { AutoApproveSettings } from "../AutoApproveSettings"

export const AutoApproveSettingsFixture = () => (
<TranslationContext.Provider
value={{
t: (key) => (key === "settings:autoApprove.followupQuestions.timeoutDisabled" ? "Disabled" : key),
i18n: i18next,
}}>
<ExtensionStateContextProvider
initialState={{ autoApprovalEnabled: false, alwaysAllowFollowupQuestions: true }}>
<div className="w-[680px] bg-vscode-editor-background p-4 text-vscode-foreground">
<AutoApproveSettings
alwaysAllowFollowupQuestions
followupAutoApproveTimeoutMs={0}
setCachedStateField={() => {}}
/>
</div>
</ExtensionStateContextProvider>
</TranslationContext.Provider>
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import React from "react"

import { expect, test } from "../../../../playwright/coverage-fixture"
import { AutoApproveSettingsFixture } from "./AutoApproveSettings.visual.fixture"

test("renders follow-up timeout row with disabled state label in the VS Code dark theme", async ({ mount, page }) => {
// The full settings bundle can leave a bare Zod reference after CT tree-shaking.
await page.evaluate(() => Object.assign(globalThis, { z: undefined }))

const component = await mount(<AutoApproveSettingsFixture />)

await component.evaluate(async () => {
await document.fonts.ready
await new Promise<void>((resolve) => requestAnimationFrame(() => resolve()))
})

await expect(component).toHaveScreenshot("auto-approve-followup-timeout-disabled-dark.png")
})
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 2 additions & 1 deletion webview-ui/src/i18n/locales/ca/settings.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion webview-ui/src/i18n/locales/de/settings.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion webview-ui/src/i18n/locales/en/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,8 @@
"followupQuestions": {
"label": "Question",
"description": "Automatically select the first suggested answer for follow-up questions after the configured timeout",
"timeoutLabel": "Time to wait before auto-selecting the first answer"
"timeoutLabel": "Time to wait before auto-selecting the first answer",
"timeoutDisabled": "Disabled"
},
"execute": {
"label": "Execute",
Expand Down
3 changes: 2 additions & 1 deletion webview-ui/src/i18n/locales/es/settings.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading