From 105aef70b1d442820fccda452970258eb4769119 Mon Sep 17 00:00:00 2001 From: sam hoang Date: Sat, 31 May 2025 04:55:11 +0700 Subject: [PATCH 1/2] feat(ConcurrentFileReadsExperiment): improve logic for enabling/disabling and add tests --- .../ConcurrentFileReadsExperiment.tsx | 19 ++- .../ConcurrentFileReadsExperiment.test.tsx | 141 ++++++++++++++++++ 2 files changed, 152 insertions(+), 8 deletions(-) create mode 100644 webview-ui/src/components/settings/__tests__/ConcurrentFileReadsExperiment.test.tsx diff --git a/webview-ui/src/components/settings/ConcurrentFileReadsExperiment.tsx b/webview-ui/src/components/settings/ConcurrentFileReadsExperiment.tsx index f2190aa7d2..e203e4cf75 100644 --- a/webview-ui/src/components/settings/ConcurrentFileReadsExperiment.tsx +++ b/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={[maxConcurrentFileReads]} onValueChange={([value]) => onMaxConcurrentFileReadsChange(value)} data-testid="max-concurrent-file-reads-slider" /> - - {maxConcurrentFileReads && maxConcurrentFileReads > 1 ? maxConcurrentFileReads : 15} - + {maxConcurrentFileReads} diff --git a/webview-ui/src/components/settings/__tests__/ConcurrentFileReadsExperiment.test.tsx b/webview-ui/src/components/settings/__tests__/ConcurrentFileReadsExperiment.test.tsx new file mode 100644 index 0000000000..0c302d006e --- /dev/null +++ b/webview-ui/src/components/settings/__tests__/ConcurrentFileReadsExperiment.test.tsx @@ -0,0 +1,141 @@ +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( + , + ) + + 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( + , + ) + + 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( + , + ) + + 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( + , + ) + + 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( + , + ) + + 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( + , + ) + + // Verify initial value is displayed + expect(screen.getByText("15")).toBeInTheDocument() + + // Simulate the slider change by re-rendering with new value + rerender( + , + ) + + // Verify new value is displayed + expect(screen.getByText("50")).toBeInTheDocument() + }) +}) From 4da7b796848d1aba4f86ee150ff39ff474bbeb21 Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Fri, 30 May 2025 18:55:41 -0500 Subject: [PATCH 2/2] 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 --- .../ConcurrentFileReadsExperiment.tsx | 4 +-- .../ConcurrentFileReadsExperiment.test.tsx | 31 +++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/webview-ui/src/components/settings/ConcurrentFileReadsExperiment.tsx b/webview-ui/src/components/settings/ConcurrentFileReadsExperiment.tsx index e203e4cf75..fb1f0dc718 100644 --- a/webview-ui/src/components/settings/ConcurrentFileReadsExperiment.tsx +++ b/webview-ui/src/components/settings/ConcurrentFileReadsExperiment.tsx @@ -55,11 +55,11 @@ export const ConcurrentFileReadsExperiment = ({ min={2} max={100} step={1} - value={[maxConcurrentFileReads]} + value={[Math.max(2, maxConcurrentFileReads)]} onValueChange={([value]) => onMaxConcurrentFileReadsChange(value)} data-testid="max-concurrent-file-reads-slider" /> - {maxConcurrentFileReads} + {Math.max(2, maxConcurrentFileReads)} diff --git a/webview-ui/src/components/settings/__tests__/ConcurrentFileReadsExperiment.test.tsx b/webview-ui/src/components/settings/__tests__/ConcurrentFileReadsExperiment.test.tsx index 0c302d006e..590a072ee9 100644 --- a/webview-ui/src/components/settings/__tests__/ConcurrentFileReadsExperiment.test.tsx +++ b/webview-ui/src/components/settings/__tests__/ConcurrentFileReadsExperiment.test.tsx @@ -138,4 +138,35 @@ describe("ConcurrentFileReadsExperiment", () => { // Verify new value is displayed expect(screen.getByText("50")).toBeInTheDocument() }) + + it("should display minimum value of 2 when maxConcurrentFileReads is less than 2", () => { + render( + , + ) + + // 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( + , + ) + + const checkbox = screen.getByTestId("concurrent-file-reads-checkbox") + fireEvent.click(checkbox) + + expect(mockOnEnabledChange).toHaveBeenCalledWith(true) + expect(mockOnMaxConcurrentFileReadsChange).toHaveBeenCalledWith(15) + }) })