feat(ConcurrentFileReadsExperiment): improve logic for enabling/disabling and add tests (#4154)

* feat(ConcurrentFileReadsExperiment): improve logic for enabling/disabling and add tests

* fix: ensure slider always displays valid value (min 2) for concurrent file reads

- Fix issue where slider could display values below minimum when enabled with maxConcurrentFileReads <= 1
- Add test cases for edge cases (value 0 and 1)
- Ensure UI always shows at least the minimum value of 2

---------

Co-authored-by: Daniel Riccio <[email protected]>

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

@@ -18,9 +18,16 @@ export const ConcurrentFileReadsExperiment = ({
 	const { t } = useAppTranslation()
 
 	const handleChange = (value: boolean) => {
-		// Set to 1 if disabling to reset the setting
-		if (!value) onMaxConcurrentFileReadsChange(1)
 		onEnabledChange(value)
+		// Set to 1 if disabling to reset the setting
+		if (!value) {
+			onMaxConcurrentFileReadsChange(1)
+		} else {
+			// When enabling, ensure we have a valid value > 1
+			if (!maxConcurrentFileReads || maxConcurrentFileReads <= 1) {
+				onMaxConcurrentFileReadsChange(15)
+			}
+		}
 	}
 
 	return (
@@ -48,15 +55,11 @@ export const ConcurrentFileReadsExperiment = ({
 								min={2}
 								max={100}
 								step={1}
-								value={[
-									maxConcurrentFileReads && maxConcurrentFileReads > 1 ? maxConcurrentFileReads : 15,
-								]}
+								value={[Math.max(2, maxConcurrentFileReads)]}
 								onValueChange={([value]) => onMaxConcurrentFileReadsChange(value)}
 								data-testid="max-concurrent-file-reads-slider"
 							/>
-							<span className="w-10 text-sm">
-								{maxConcurrentFileReads && maxConcurrentFileReads > 1 ? maxConcurrentFileReads : 15}
-							</span>
+							<span className="w-10 text-sm">{Math.max(2, maxConcurrentFileReads)}</span>
 						</div>
 					</div>
 				</div>

webview-ui/src/components/settings/__tests__/ConcurrentFileReadsExperiment.test.tsx

@@ -0,0 +1,172 @@
+import { render, screen, fireEvent } from "@testing-library/react"
+import { ConcurrentFileReadsExperiment } from "../ConcurrentFileReadsExperiment"
+
+// Mock the translation hook
+jest.mock("@/i18n/TranslationContext", () => ({
+	useAppTranslation: () => ({
+		t: (key: string) => key,
+	}),
+}))
+
+// Mock ResizeObserver which is used by the Slider component
+global.ResizeObserver = jest.fn().mockImplementation(() => ({
+	observe: jest.fn(),
+	unobserve: jest.fn(),
+	disconnect: jest.fn(),
+}))
+
+describe("ConcurrentFileReadsExperiment", () => {
+	const mockOnEnabledChange = jest.fn()
+	const mockOnMaxConcurrentFileReadsChange = jest.fn()
+
+	beforeEach(() => {
+		jest.clearAllMocks()
+	})
+
+	it("should render with disabled state", () => {
+		render(
+			<ConcurrentFileReadsExperiment
+				enabled={false}
+				onEnabledChange={mockOnEnabledChange}
+				maxConcurrentFileReads={1}
+				onMaxConcurrentFileReadsChange={mockOnMaxConcurrentFileReadsChange}
+			/>,
+		)
+
+		const checkbox = screen.getByTestId("concurrent-file-reads-checkbox")
+		expect(checkbox).not.toBeChecked()
+
+		// Slider should not be visible when disabled
+		expect(screen.queryByTestId("max-concurrent-file-reads-slider")).not.toBeInTheDocument()
+	})
+
+	it("should render with enabled state", () => {
+		render(
+			<ConcurrentFileReadsExperiment
+				enabled={true}
+				onEnabledChange={mockOnEnabledChange}
+				maxConcurrentFileReads={20}
+				onMaxConcurrentFileReadsChange={mockOnMaxConcurrentFileReadsChange}
+			/>,
+		)
+
+		const checkbox = screen.getByTestId("concurrent-file-reads-checkbox")
+		expect(checkbox).toBeChecked()
+
+		// Slider should be visible when enabled
+		expect(screen.getByTestId("max-concurrent-file-reads-slider")).toBeInTheDocument()
+		expect(screen.getByText("20")).toBeInTheDocument()
+	})
+
+	it("should set maxConcurrentFileReads to 15 when enabling from disabled state", () => {
+		render(
+			<ConcurrentFileReadsExperiment
+				enabled={false}
+				onEnabledChange={mockOnEnabledChange}
+				maxConcurrentFileReads={1}
+				onMaxConcurrentFileReadsChange={mockOnMaxConcurrentFileReadsChange}
+			/>,
+		)
+
+		const checkbox = screen.getByTestId("concurrent-file-reads-checkbox")
+		fireEvent.click(checkbox)
+
+		expect(mockOnEnabledChange).toHaveBeenCalledWith(true)
+		expect(mockOnMaxConcurrentFileReadsChange).toHaveBeenCalledWith(15)
+	})
+
+	it("should set maxConcurrentFileReads to 1 when disabling", () => {
+		render(
+			<ConcurrentFileReadsExperiment
+				enabled={true}
+				onEnabledChange={mockOnEnabledChange}
+				maxConcurrentFileReads={25}
+				onMaxConcurrentFileReadsChange={mockOnMaxConcurrentFileReadsChange}
+			/>,
+		)
+
+		const checkbox = screen.getByTestId("concurrent-file-reads-checkbox")
+		fireEvent.click(checkbox)
+
+		expect(mockOnEnabledChange).toHaveBeenCalledWith(false)
+		expect(mockOnMaxConcurrentFileReadsChange).toHaveBeenCalledWith(1)
+	})
+
+	it("should not change maxConcurrentFileReads when enabling if already > 1", () => {
+		render(
+			<ConcurrentFileReadsExperiment
+				enabled={false}
+				onEnabledChange={mockOnEnabledChange}
+				maxConcurrentFileReads={30}
+				onMaxConcurrentFileReadsChange={mockOnMaxConcurrentFileReadsChange}
+			/>,
+		)
+
+		const checkbox = screen.getByTestId("concurrent-file-reads-checkbox")
+		fireEvent.click(checkbox)
+
+		expect(mockOnEnabledChange).toHaveBeenCalledWith(true)
+		// Should not call onMaxConcurrentFileReadsChange since value is already > 1
+		expect(mockOnMaxConcurrentFileReadsChange).not.toHaveBeenCalled()
+	})
+
+	it("should update value when slider changes", () => {
+		// Since the Slider component doesn't render a standard input,
+		// we'll test the component's interaction through its props
+		const { rerender } = render(
+			<ConcurrentFileReadsExperiment
+				enabled={true}
+				onEnabledChange={mockOnEnabledChange}
+				maxConcurrentFileReads={15}
+				onMaxConcurrentFileReadsChange={mockOnMaxConcurrentFileReadsChange}
+			/>,
+		)
+
+		// Verify initial value is displayed
+		expect(screen.getByText("15")).toBeInTheDocument()
+
+		// Simulate the slider change by re-rendering with new value
+		rerender(
+			<ConcurrentFileReadsExperiment
+				enabled={true}
+				onEnabledChange={mockOnEnabledChange}
+				maxConcurrentFileReads={50}
+				onMaxConcurrentFileReadsChange={mockOnMaxConcurrentFileReadsChange}
+			/>,
+		)
+
+		// Verify new value is displayed
+		expect(screen.getByText("50")).toBeInTheDocument()
+	})
+
+	it("should display minimum value of 2 when maxConcurrentFileReads is less than 2", () => {
+		render(
+			<ConcurrentFileReadsExperiment
+				enabled={true}
+				onEnabledChange={mockOnEnabledChange}
+				maxConcurrentFileReads={1}
+				onMaxConcurrentFileReadsChange={mockOnMaxConcurrentFileReadsChange}
+			/>,
+		)
+
+		// Should display 2 (minimum value) instead of 1
+		expect(screen.getByText("2")).toBeInTheDocument()
+	})
+
+	it("should set maxConcurrentFileReads to 15 when enabling with value of 0", () => {
+		render(
+			<ConcurrentFileReadsExperiment
+				enabled={false}
+				onEnabledChange={mockOnEnabledChange}
+				maxConcurrentFileReads={0}
+				onMaxConcurrentFileReadsChange={mockOnMaxConcurrentFileReadsChange}
+			/>,
+		)
+
+		const checkbox = screen.getByTestId("concurrent-file-reads-checkbox")
+		fireEvent.click(checkbox)
+
+		expect(mockOnEnabledChange).toHaveBeenCalledWith(true)
+		expect(mockOnMaxConcurrentFileReadsChange).toHaveBeenCalledWith(15)
+	})
+})