From e6e1c5bbf77c2e5e18ef51d23329e2d90a804d0f Mon Sep 17 00:00:00 2001 From: Roo Code Date: Sat, 21 Feb 2026 07:17:13 +0000 Subject: [PATCH] fix: move font size telemetry to blur handler, add comprehensive tests - Move telemetry capture from onChange to onBlur in handleFontSizeMultiplierBlur so intermediate keystrokes do not emit events (addresses review comment 3e180acf39) - Add 14 new test cases covering font size input rendering, change/blur/reset handlers, clamping logic, NaN handling, and telemetry behavior (addresses review comment 4bf3aa3bb4) --- .../src/components/settings/UISettings.tsx | 10 +- .../settings/__tests__/UISettings.spec.tsx | 154 ++++++++++++++++++ 2 files changed, 159 insertions(+), 5 deletions(-) diff --git a/webview-ui/src/components/settings/UISettings.tsx b/webview-ui/src/components/settings/UISettings.tsx index 7228dc67fa..61fcf01828 100644 --- a/webview-ui/src/components/settings/UISettings.tsx +++ b/webview-ui/src/components/settings/UISettings.tsx @@ -69,11 +69,6 @@ export const UISettings = ({ // Clamp the value between 0.5 and 2 const clampedValue = Math.max(0.5, Math.min(2, numValue)) setCachedStateField("chatFontSizeMultiplier", clampedValue) - - // Track telemetry event - telemetryClient.capture("ui_settings_chat_font_size_changed", { - multiplier: clampedValue, - }) } }, [setCachedStateField], @@ -87,6 +82,11 @@ export const UISettings = ({ } else { const clampedValue = Math.max(0.5, Math.min(2, numValue)) setLocalMultiplier(clampedValue.toString()) + + // Track telemetry event on blur to capture only the user's final value + telemetryClient.capture("ui_settings_chat_font_size_changed", { + multiplier: clampedValue, + }) } }, [localMultiplier, chatFontSizeMultiplier]) diff --git a/webview-ui/src/components/settings/__tests__/UISettings.spec.tsx b/webview-ui/src/components/settings/__tests__/UISettings.spec.tsx index 8faa8f1be2..f739b3f34a 100644 --- a/webview-ui/src/components/settings/__tests__/UISettings.spec.tsx +++ b/webview-ui/src/components/settings/__tests__/UISettings.spec.tsx @@ -2,6 +2,14 @@ import { render, fireEvent, waitFor } from "@testing-library/react" import { describe, it, expect, vi } from "vitest" import { UISettings } from "../UISettings" +// Mock telemetryClient +const mockCapture = vi.fn() +vi.mock("@/utils/TelemetryClient", () => ({ + telemetryClient: { + capture: (eventName: string, properties?: Record) => mockCapture(eventName, properties), + }, +})) + describe("UISettings", () => { const defaultProps = { reasoningBlockCollapsed: false, @@ -10,6 +18,10 @@ describe("UISettings", () => { setCachedStateField: vi.fn(), } + beforeEach(() => { + vi.clearAllMocks() + }) + it("renders the collapse thinking checkbox", () => { const { getByTestId } = render() const checkbox = getByTestId("collapse-thinking-checkbox") @@ -42,4 +54,146 @@ describe("UISettings", () => { rerender() expect(checkbox.checked).toBe(true) }) + + describe("Chat Font Size Multiplier", () => { + it("renders the font size input with the correct default value", () => { + const { getByTestId } = render() + const input = getByTestId("chat-font-size-input") as HTMLInputElement + expect(input).toBeTruthy() + expect(input.value).toBe("1") + }) + + it("renders the reset button", () => { + const { getByTestId } = render() + const resetButton = getByTestId("chat-font-size-reset-button") + expect(resetButton).toBeTruthy() + }) + + it("displays custom multiplier value from props", () => { + const { getByTestId } = render() + const input = getByTestId("chat-font-size-input") as HTMLInputElement + expect(input.value).toBe("1.5") + }) + + it("calls setCachedStateField on change with a valid value", () => { + const setCachedStateField = vi.fn() + const { getByTestId } = render() + const input = getByTestId("chat-font-size-input") + + fireEvent.change(input, { target: { value: "1.5" } }) + + expect(setCachedStateField).toHaveBeenCalledWith("chatFontSizeMultiplier", 1.5) + }) + + it("does not call setCachedStateField on change with NaN input", () => { + const setCachedStateField = vi.fn() + const { getByTestId } = render() + const input = getByTestId("chat-font-size-input") + + fireEvent.change(input, { target: { value: "abc" } }) + + expect(setCachedStateField).not.toHaveBeenCalledWith("chatFontSizeMultiplier", expect.anything()) + }) + + it("clamps values below 0.5 to 0.5 on change", () => { + const setCachedStateField = vi.fn() + const { getByTestId } = render() + const input = getByTestId("chat-font-size-input") + + fireEvent.change(input, { target: { value: "0.1" } }) + + expect(setCachedStateField).toHaveBeenCalledWith("chatFontSizeMultiplier", 0.5) + }) + + it("clamps values above 2 to 2 on change", () => { + const setCachedStateField = vi.fn() + const { getByTestId } = render() + const input = getByTestId("chat-font-size-input") + + fireEvent.change(input, { target: { value: "5" } }) + + expect(setCachedStateField).toHaveBeenCalledWith("chatFontSizeMultiplier", 2) + }) + + it("normalizes the display value on blur for a valid value", () => { + const { getByTestId } = render() + const input = getByTestId("chat-font-size-input") as HTMLInputElement + + fireEvent.change(input, { target: { value: "0.3" } }) + fireEvent.blur(input) + + // Should be clamped to 0.5 in the display + expect(input.value).toBe("0.5") + }) + + it("resets display value to prop on blur with NaN input", () => { + const { getByTestId } = render() + const input = getByTestId("chat-font-size-input") as HTMLInputElement + + fireEvent.change(input, { target: { value: "abc" } }) + fireEvent.blur(input) + + // Should reset to the prop value + expect(input.value).toBe("1.2") + }) + + it("fires telemetry on blur, not on change", () => { + const { getByTestId } = render() + const input = getByTestId("chat-font-size-input") + + fireEvent.change(input, { target: { value: "1.5" } }) + + // Telemetry should NOT have fired on change + expect(mockCapture).not.toHaveBeenCalledWith("ui_settings_chat_font_size_changed", expect.anything()) + + fireEvent.blur(input) + + // Telemetry should fire on blur with the clamped value + expect(mockCapture).toHaveBeenCalledWith("ui_settings_chat_font_size_changed", { + multiplier: 1.5, + }) + }) + + it("does not fire telemetry on blur with NaN input", () => { + const { getByTestId } = render() + const input = getByTestId("chat-font-size-input") + + fireEvent.change(input, { target: { value: "abc" } }) + fireEvent.blur(input) + + expect(mockCapture).not.toHaveBeenCalledWith("ui_settings_chat_font_size_changed", expect.anything()) + }) + + it("resets font size to 1 when reset button is clicked", () => { + const setCachedStateField = vi.fn() + const { getByTestId } = render( + , + ) + const input = getByTestId("chat-font-size-input") as HTMLInputElement + const resetButton = getByTestId("chat-font-size-reset-button") + + fireEvent.click(resetButton) + + expect(setCachedStateField).toHaveBeenCalledWith("chatFontSizeMultiplier", 1) + expect(input.value).toBe("1") + }) + + it("fires reset telemetry when reset button is clicked", () => { + const { getByTestId } = render() + const resetButton = getByTestId("chat-font-size-reset-button") + + fireEvent.click(resetButton) + + expect(mockCapture).toHaveBeenCalledWith("ui_settings_chat_font_size_reset", {}) + }) + + it("syncs local state when chatFontSizeMultiplier prop changes", () => { + const { getByTestId, rerender } = render() + const input = getByTestId("chat-font-size-input") as HTMLInputElement + expect(input.value).toBe("1") + + rerender() + expect(input.value).toBe("1.8") + }) + }) })