From 6af93241b6761b3eba7b41616efae6007f3320fb Mon Sep 17 00:00:00 2001 From: Bruno Bergher Date: Mon, 15 Sep 2025 13:00:16 +0100 Subject: [PATCH 1/2] Makes upsell dialog visibility available outside the component to fix an issue where would be incorrectly hidden. --- webview-ui/src/components/chat/ChatView.tsx | 12 +- webview-ui/src/components/chat/TaskHeader.tsx | 6 +- .../components/common/DismissibleUpsell.tsx | 69 ++-- .../__tests__/DismissibleUpsell.spec.tsx | 289 ++++++------- webview-ui/src/constants/upsellIds.ts | 17 + .../src/context/DismissedUpsellsContext.tsx | 111 +++++ .../DismissedUpsellsContext.spec.tsx | 385 ++++++++++++++++++ .../__tests__/useUpsellVisibility.spec.tsx | 299 ++++++++++++++ webview-ui/src/hooks/useUpsellVisibility.ts | 95 +++++ 9 files changed, 1072 insertions(+), 211 deletions(-) create mode 100644 webview-ui/src/constants/upsellIds.ts create mode 100644 webview-ui/src/context/DismissedUpsellsContext.tsx create mode 100644 webview-ui/src/context/__tests__/DismissedUpsellsContext.spec.tsx create mode 100644 webview-ui/src/hooks/__tests__/useUpsellVisibility.spec.tsx create mode 100644 webview-ui/src/hooks/useUpsellVisibility.ts diff --git a/webview-ui/src/components/chat/ChatView.tsx b/webview-ui/src/components/chat/ChatView.tsx index d358c68f1c..c647750516 100644 --- a/webview-ui/src/components/chat/ChatView.tsx +++ b/webview-ui/src/components/chat/ChatView.tsx @@ -40,7 +40,6 @@ import RooTips from "@src/components/welcome/RooTips" import { StandardTooltip } from "@src/components/ui" import { useAutoApprovalState } from "@src/hooks/useAutoApprovalState" import { useAutoApprovalToggles } from "@src/hooks/useAutoApprovalToggles" -import { CloudUpsellDialog } from "@src/components/cloud/CloudUpsellDialog" import TelemetryBanner from "../common/TelemetryBanner" import VersionIndicator from "../common/VersionIndicator" @@ -55,9 +54,12 @@ import SystemPromptWarning from "./SystemPromptWarning" import ProfileViolationWarning from "./ProfileViolationWarning" import { CheckpointWarning } from "./CheckpointWarning" import { QueuedMessages } from "./QueuedMessages" +import { Cloud } from "lucide-react" + +import { CloudUpsellDialog } from "@src/components/cloud/CloudUpsellDialog" import DismissibleUpsell from "../common/DismissibleUpsell" import { useCloudUpsell } from "@src/hooks/useCloudUpsell" -import { Cloud } from "lucide-react" +import { useUpsellVisibility, UPSELL_IDS } from "@/hooks/useUpsellVisibility" export interface ChatViewProps { isHidden: boolean @@ -1772,6 +1774,8 @@ const ChatViewComponent: React.ForwardRefRenderFunction
- {cloudIsAuthenticated || taskHistory.length < 4 ? ( + {cloudIsAuthenticated || (tasks.length === 0 && !isCloudUpsellVisible) ? ( ) : ( <> } onClick={() => openUpsell()} dismissOnClick={false} diff --git a/webview-ui/src/components/chat/TaskHeader.tsx b/webview-ui/src/components/chat/TaskHeader.tsx index 6164294722..b4b03ac1a8 100644 --- a/webview-ui/src/components/chat/TaskHeader.tsx +++ b/webview-ui/src/components/chat/TaskHeader.tsx @@ -2,7 +2,6 @@ import { memo, useEffect, useRef, useState } from "react" import { useTranslation } from "react-i18next" import { useCloudUpsell } from "@src/hooks/useCloudUpsell" import { CloudUpsellDialog } from "@src/components/cloud/CloudUpsellDialog" -import DismissibleUpsell from "@src/components/common/DismissibleUpsell" import { FoldVertical, ChevronUp, ChevronDown } from "lucide-react" import prettyBytes from "pretty-bytes" @@ -24,6 +23,9 @@ import { ContextWindowProgress } from "./ContextWindowProgress" import { Mention } from "./Mention" import { TodoListDisplay } from "./TodoListDisplay" +import DismissibleUpsell from "@src/components/common/DismissibleUpsell" +import { UPSELL_IDS } from "@/constants/upsellIds" + export interface TaskHeaderProps { task: ClineMessage tokensIn: number @@ -103,7 +105,7 @@ const TaskHeader = ({
{showLongRunningTaskMessage && !isTaskComplete && ( openUpsell()} dismissOnClick={false} variant="banner"> diff --git a/webview-ui/src/components/common/DismissibleUpsell.tsx b/webview-ui/src/components/common/DismissibleUpsell.tsx index 2eefb89972..073b1d0c74 100644 --- a/webview-ui/src/components/common/DismissibleUpsell.tsx +++ b/webview-ui/src/components/common/DismissibleUpsell.tsx @@ -1,6 +1,6 @@ -import { memo, ReactNode, useEffect, useState, useRef } from "react" -import { vscode } from "@src/utils/vscode" +import { memo, ReactNode } from "react" import { useAppTranslation } from "@src/i18n/TranslationContext" +import { DismissedUpsellsProvider, useDismissedUpsells } from "@src/context/DismissedUpsellsContext" interface DismissibleUpsellProps { /** Required unique identifier for this upsell */ @@ -32,7 +32,8 @@ const DismissIcon = () => ( ) -const DismissibleUpsell = memo( +// Internal component that uses the context +const DismissibleUpsellInternal = memo( ({ upsellId, className, @@ -44,55 +45,21 @@ const DismissibleUpsell = memo( dismissOnClick = false, }: DismissibleUpsellProps) => { const { t } = useAppTranslation() - const [isVisible, setIsVisible] = useState(false) - const isMountedRef = useRef(true) + const { isUpsellVisible, dismissUpsell, isLoading } = useDismissedUpsells() - useEffect(() => { - // Track mounted state - isMountedRef.current = true + // Check if this upsell is visible + const isVisible = isUpsellVisible(upsellId) - // Request the current list of dismissed upsells from the extension - vscode.postMessage({ type: "getDismissedUpsells" }) - - // Listen for the response - const handleMessage = (event: MessageEvent) => { - // Only update state if component is still mounted - if (!isMountedRef.current) return - - const message = event.data - // Add null/undefined check for message - if (message && message.type === "dismissedUpsells" && Array.isArray(message.list)) { - // Check if this upsell has been dismissed - if (!message.list.includes(upsellId)) { - setIsVisible(true) - } - } - } - - window.addEventListener("message", handleMessage) - return () => { - isMountedRef.current = false - window.removeEventListener("message", handleMessage) - } - }, [upsellId]) - - const handleDismiss = async () => { - // First notify the extension to persist the dismissal - // This ensures the message is sent even if the component unmounts quickly - vscode.postMessage({ - type: "dismissUpsell", - upsellId: upsellId, - }) - - // Then hide the upsell - setIsVisible(false) + const handleDismiss = () => { + // Dismiss the upsell through the context + dismissUpsell(upsellId) // Call the optional callback onDismiss?.() } - // Don't render if not visible - if (!isVisible) { + // Don't render if not visible or still loading + if (!isVisible || isLoading) { return null } @@ -107,6 +74,7 @@ const DismissibleUpsell = memo( button: "text-vscode-notifications-foreground", }, } + // Build container classes based on variant and presence of click handler const containerClasses = [ "relative flex items-start justify-between gap-2", @@ -158,6 +126,17 @@ const DismissibleUpsell = memo( }, ) +DismissibleUpsellInternal.displayName = "DismissibleUpsellInternal" + +// Wrapper component that provides the context +const DismissibleUpsell = memo((props: DismissibleUpsellProps) => { + return ( + + + + ) +}) + DismissibleUpsell.displayName = "DismissibleUpsell" export default DismissibleUpsell diff --git a/webview-ui/src/components/common/__tests__/DismissibleUpsell.spec.tsx b/webview-ui/src/components/common/__tests__/DismissibleUpsell.spec.tsx index 3af66dfdf1..393cce6f85 100644 --- a/webview-ui/src/components/common/__tests__/DismissibleUpsell.spec.tsx +++ b/webview-ui/src/components/common/__tests__/DismissibleUpsell.spec.tsx @@ -1,6 +1,7 @@ import { render, screen, fireEvent, waitFor, act } from "@testing-library/react" import { describe, it, expect, vi, beforeEach, afterEach } from "vitest" import DismissibleUpsell from "../DismissibleUpsell" +import React from "react" // Mock the vscode API const mockPostMessage = vi.fn() @@ -24,34 +25,59 @@ vi.mock("@src/i18n/TranslationContext", () => ({ })) describe("DismissibleUpsell", () => { + let messageHandler: ((event: MessageEvent) => void) | null = null + beforeEach(() => { mockPostMessage.mockClear() vi.clearAllTimers() + + // Capture the message event handler + window.addEventListener = vi.fn((event, handler) => { + if (event === "message") { + messageHandler = handler as (event: MessageEvent) => void + } + }) + + window.removeEventListener = vi.fn() }) afterEach(() => { vi.clearAllTimers() + messageHandler = null }) // Helper function to make the component visible const makeUpsellVisible = () => { - const messageEvent = new MessageEvent("message", { - data: { - type: "dismissedUpsells", - list: [], // Empty list means no upsells are dismissed - }, + act(() => { + messageHandler?.({ + data: { + type: "dismissedUpsells", + list: [], // Empty list means no upsells are dismissed + }, + } as MessageEvent) }) - window.dispatchEvent(messageEvent) } - it("renders children content", async () => { + // Helper function to mark upsell as dismissed + const makeUpsellDismissed = (upsellIds: string[]) => { + act(() => { + messageHandler?.({ + data: { + type: "dismissedUpsells", + list: upsellIds, + }, + } as MessageEvent) + }) + } + + it("renders children content when visible", async () => { render(
Test content
, ) - // Component starts hidden, make it visible + // Component starts hidden (loading), make it visible makeUpsellVisible() // Wait for component to become visible @@ -60,7 +86,7 @@ describe("DismissibleUpsell", () => { }) }) - it("requests dismissed upsells list on mount", () => { + it("requests dismissed upsells list on mount via context", () => { render(
Test content
@@ -92,7 +118,7 @@ describe("DismissibleUpsell", () => { const dismissButton = screen.getByRole("button", { name: /dismiss/i }) fireEvent.click(dismissButton) - // Check that the dismiss message was sent BEFORE hiding + // Check that the dismiss message was sent expect(mockPostMessage).toHaveBeenCalledWith({ type: "dismissUpsell", upsellId: "test-upsell", @@ -114,17 +140,8 @@ describe("DismissibleUpsell", () => { , ) - // Component starts hidden by default - expect(container.firstChild).toBeNull() - // Simulate receiving a message that this upsell is dismissed - const messageEvent = new MessageEvent("message", { - data: { - type: "dismissedUpsells", - list: ["test-upsell", "other-upsell"], - }, - }) - window.dispatchEvent(messageEvent) + makeUpsellDismissed(["test-upsell", "other-upsell"]) // Check that the component remains hidden await waitFor(() => { @@ -140,15 +157,9 @@ describe("DismissibleUpsell", () => { ) // Simulate receiving a message that doesn't include this upsell - const messageEvent = new MessageEvent("message", { - data: { - type: "dismissedUpsells", - list: ["other-upsell"], - }, - }) - window.dispatchEvent(messageEvent) + makeUpsellDismissed(["other-upsell"]) - // Check that the component is still visible + // Check that the component is visible await waitFor(() => { expect(screen.getByText("Test content")).toBeInTheDocument() }) @@ -192,7 +203,6 @@ describe("DismissibleUpsell", () => { expect(dismissButton).toHaveAttribute("title", "Dismiss and don't show again") }) - // New edge case tests it("handles multiple rapid dismissals of the same component", async () => { const onDismiss = vi.fn() render( @@ -216,141 +226,16 @@ describe("DismissibleUpsell", () => { fireEvent.click(dismissButton) fireEvent.click(dismissButton) - // Should only send one message - expect(mockPostMessage).toHaveBeenCalledTimes(2) // 1 for getDismissedUpsells, 1 for dismissUpsell - expect(mockPostMessage).toHaveBeenCalledWith({ - type: "dismissUpsell", - upsellId: "test-upsell", - }) + // Should only send one dismiss message (plus initial getDismissedUpsells) + const dismissCalls = mockPostMessage.mock.calls.filter( + (call) => call[0].type === "dismissUpsell" && call[0].upsellId === "test-upsell", + ) + expect(dismissCalls.length).toBe(1) // Callback should only be called once expect(onDismiss).toHaveBeenCalledTimes(1) }) - it("does not update state after component unmounts", async () => { - const { unmount } = render( - -
Test content
- , - ) - - // Unmount the component - unmount() - - // Simulate receiving a message after unmount - const messageEvent = new MessageEvent("message", { - data: { - type: "dismissedUpsells", - list: ["test-upsell"], - }, - }) - - // This should not cause any errors - act(() => { - window.dispatchEvent(messageEvent) - }) - - // No errors should be thrown - expect(true).toBe(true) - }) - - it("handles invalid/malformed messages gracefully", async () => { - render( - -
Test content
- , - ) - - // First make it visible - makeUpsellVisible() - - // Wait for component to be visible - await waitFor(() => { - expect(screen.getByText("Test content")).toBeInTheDocument() - }) - - // Send various malformed messages - const malformedMessages = [ - { type: "dismissedUpsells", list: null }, - { type: "dismissedUpsells", list: "not-an-array" }, - { type: "dismissedUpsells" }, // missing list - { type: "wrongType", list: ["test-upsell"] }, - null, - undefined, - "string-message", - ] - - malformedMessages.forEach((data) => { - const messageEvent = new MessageEvent("message", { data }) - window.dispatchEvent(messageEvent) - }) - - // Component should still be visible - expect(screen.getByText("Test content")).toBeInTheDocument() - }) - - it("ensures message is sent before component unmounts on dismiss", async () => { - const { unmount } = render( - -
Test content
- , - ) - - // Make component visible - makeUpsellVisible() - - // Wait for component to be visible - await waitFor(() => { - expect(screen.getByText("Test content")).toBeInTheDocument() - }) - - const dismissButton = screen.getByRole("button", { name: /dismiss/i }) - fireEvent.click(dismissButton) - - // Message should be sent immediately - expect(mockPostMessage).toHaveBeenCalledWith({ - type: "dismissUpsell", - upsellId: "test-upsell", - }) - - // Unmount immediately after clicking - unmount() - - // Message was already sent before unmount - expect(mockPostMessage).toHaveBeenCalledWith({ - type: "dismissUpsell", - upsellId: "test-upsell", - }) - }) - - it("uses separate id and className props correctly", async () => { - const { container } = render( - -
Test content
- , - ) - - // Make component visible - makeUpsellVisible() - - // Wait for component to be visible - await waitFor(() => { - expect(container.firstChild).not.toBeNull() - }) - - // className should be applied to the container - expect(container.firstChild).toHaveClass("styling-class") - - // When dismissed, should use the id, not className - const dismissButton = screen.getByRole("button", { name: /dismiss/i }) - fireEvent.click(dismissButton) - - expect(mockPostMessage).toHaveBeenCalledWith({ - type: "dismissUpsell", - upsellId: "unique-id", - }) - }) - it("calls onClick when the container is clicked", async () => { const onClick = vi.fn() render( @@ -425,6 +310,14 @@ describe("DismissibleUpsell", () => { , ) + // Make sure it's still visible after re-render + makeUpsellVisible() + + // Wait for component to be visible again + await waitFor(() => { + expect(container.firstChild).not.toBeNull() + }) + // Should not have cursor-pointer when onClick is not provided expect(container.firstChild).not.toHaveClass("cursor-pointer") }) @@ -526,7 +419,10 @@ describe("DismissibleUpsell", () => { expect(onClick).toHaveBeenCalledTimes(1) expect(onDismiss).not.toHaveBeenCalled() - expect(mockPostMessage).not.toHaveBeenCalledWith(expect.objectContaining({ type: "dismissUpsell" })) + // Should not dismiss the upsell + const dismissCalls = mockPostMessage.mock.calls.filter((call) => call[0].type === "dismissUpsell") + expect(dismissCalls.length).toBe(0) + expect(screen.getByText("Test content")).toBeInTheDocument() }) @@ -554,4 +450,77 @@ describe("DismissibleUpsell", () => { expect(onDismiss).not.toHaveBeenCalled() expect(screen.getByText("Test content")).toBeInTheDocument() }) + + it("renders icon when provided", async () => { + const TestIcon = () => Icon + render( + }> +
Test content
+ , + ) + + // Make component visible + makeUpsellVisible() + + // Wait for component to be visible + await waitFor(() => { + expect(screen.getByText("Test content")).toBeInTheDocument() + expect(screen.getByTestId("test-icon")).toBeInTheDocument() + }) + }) + + it("applies correct variant classes", async () => { + const { container, rerender } = render( + +
Test content
+ , + ) + + // Make component visible + makeUpsellVisible() + + // Wait for component to be visible + await waitFor(() => { + expect(container.firstChild).not.toBeNull() + }) + + // Should have banner variant classes + expect(container.firstChild).toHaveClass("bg-vscode-badge-background/80") + expect(container.firstChild).toHaveClass("text-vscode-badge-foreground") + + // Re-render with default variant + rerender( + +
Test content
+ , + ) + + // Make the new upsell visible + makeUpsellVisible() + + // Should have default variant classes + await waitFor(() => { + expect(container.firstChild).toHaveClass("bg-vscode-notifications-background") + expect(container.firstChild).toHaveClass("text-vscode-notifications-foreground") + }) + }) + + it("does not render while context is loading", async () => { + const { container } = render( + +
Test content
+ , + ) + + // Component should not render while loading + expect(container.firstChild).toBeNull() + + // Send dismissed list to complete loading + makeUpsellVisible() + + // Now component should be visible + await waitFor(() => { + expect(screen.getByText("Test content")).toBeInTheDocument() + }) + }) }) diff --git a/webview-ui/src/constants/upsellIds.ts b/webview-ui/src/constants/upsellIds.ts new file mode 100644 index 0000000000..f1656fcb14 --- /dev/null +++ b/webview-ui/src/constants/upsellIds.ts @@ -0,0 +1,17 @@ +/** + * Constants for all upsell IDs used in the application. + * Using constants ensures type safety and prevents typos. + */ + +export const UPSELL_IDS = { + TASK_LIST: "taskList", // Cloud upsell in the home page task list + LONG_RUNNING_TASK: "longRunningTask", // Cloud upsell when a task takes a while +} as const + +// Type for all valid upsell IDs +export type UpsellId = (typeof UPSELL_IDS)[keyof typeof UPSELL_IDS] + +// Helper to validate if a string is a valid upsell ID +export function isValidUpsellId(id: string): id is UpsellId { + return Object.values(UPSELL_IDS).includes(id as UpsellId) +} diff --git a/webview-ui/src/context/DismissedUpsellsContext.tsx b/webview-ui/src/context/DismissedUpsellsContext.tsx new file mode 100644 index 0000000000..51802bc6a8 --- /dev/null +++ b/webview-ui/src/context/DismissedUpsellsContext.tsx @@ -0,0 +1,111 @@ +import React, { createContext, useContext, useEffect, useState, useCallback, ReactNode } from "react" +import { vscode } from "@src/utils/vscode" +import { UpsellId } from "@src/constants/upsellIds" + +interface DismissedUpsellsContextType { + /** List of dismissed upsell IDs */ + dismissedUpsells: string[] + /** Check if a specific upsell is visible (not dismissed) */ + isUpsellVisible: (upsellId: UpsellId | string) => boolean + /** Dismiss an upsell */ + dismissUpsell: (upsellId: UpsellId | string) => void + /** Check if the context is loading initial data */ + isLoading: boolean + /** Force refresh dismissed upsells from extension */ + refreshDismissedUpsells: () => void +} + +const DismissedUpsellsContext = createContext(undefined) + +interface DismissedUpsellsProviderProps { + children: ReactNode +} + +export const DismissedUpsellsProvider: React.FC = ({ children }) => { + const [dismissedUpsells, setDismissedUpsells] = useState([]) + const [isLoading, setIsLoading] = useState(true) + + // Request dismissed upsells from extension + const refreshDismissedUpsells = useCallback(() => { + vscode.postMessage({ type: "getDismissedUpsells" }) + }, []) + + // Check if an upsell is visible + const isUpsellVisible = useCallback( + (upsellId: UpsellId | string): boolean => { + return !dismissedUpsells.includes(upsellId) + }, + [dismissedUpsells], + ) + + // Dismiss an upsell + const dismissUpsell = useCallback( + (upsellId: UpsellId | string) => { + if (!dismissedUpsells.includes(upsellId)) { + // Optimistically update local state + setDismissedUpsells((prev) => [...prev, upsellId]) + + // Send dismiss message to extension + vscode.postMessage({ + type: "dismissUpsell", + upsellId: upsellId, + }) + } + }, + [dismissedUpsells], + ) + + // Listen for messages from extension + useEffect(() => { + const handleMessage = (event: MessageEvent) => { + const message = event.data + + if (message && message.type === "dismissedUpsells" && Array.isArray(message.list)) { + setDismissedUpsells(message.list) + setIsLoading(false) + } + } + + window.addEventListener("message", handleMessage) + + // Request initial dismissed upsells list + refreshDismissedUpsells() + + return () => { + window.removeEventListener("message", handleMessage) + } + }, [refreshDismissedUpsells]) + + const value: DismissedUpsellsContextType = { + dismissedUpsells, + isUpsellVisible, + dismissUpsell, + isLoading, + refreshDismissedUpsells, + } + + return {children} +} + +/** + * Hook to use the dismissed upsells context + * @throws Error if used outside of DismissedUpsellsProvider + */ +export const useDismissedUpsells = (): DismissedUpsellsContextType => { + const context = useContext(DismissedUpsellsContext) + + if (!context) { + throw new Error("useDismissedUpsells must be used within a DismissedUpsellsProvider") + } + + return context +} + +/** + * Helper hook to check if a specific upsell is visible + * This is a convenience wrapper around useDismissedUpsells + */ +export const useIsUpsellVisible = (upsellId: UpsellId | string): boolean => { + const { isUpsellVisible } = useDismissedUpsells() + return isUpsellVisible(upsellId) +} diff --git a/webview-ui/src/context/__tests__/DismissedUpsellsContext.spec.tsx b/webview-ui/src/context/__tests__/DismissedUpsellsContext.spec.tsx new file mode 100644 index 0000000000..87f48144c7 --- /dev/null +++ b/webview-ui/src/context/__tests__/DismissedUpsellsContext.spec.tsx @@ -0,0 +1,385 @@ +import { render, screen, act, waitFor, renderHook } from "@testing-library/react" +import { DismissedUpsellsProvider, useDismissedUpsells, useIsUpsellVisible } from "../DismissedUpsellsContext" +import { vscode } from "@src/utils/vscode" +import { UPSELL_IDS } from "@src/constants/upsellIds" +import React from "react" + +// Mock vscode +vi.mock("@src/utils/vscode", () => ({ + vscode: { + postMessage: vi.fn(), + }, +})) + +describe("DismissedUpsellsContext", () => { + let messageHandler: ((event: MessageEvent) => void) | null = null + + beforeEach(() => { + vi.clearAllMocks() + + // Capture the message event handler + window.addEventListener = vi.fn((event, handler) => { + if (event === "message") { + messageHandler = handler as (event: MessageEvent) => void + } + }) + + window.removeEventListener = vi.fn() + }) + + afterEach(() => { + messageHandler = null + }) + + describe("DismissedUpsellsProvider", () => { + it("should request dismissed upsells on mount", () => { + render( + +
Test
+ , + ) + + expect(vscode.postMessage).toHaveBeenCalledWith({ type: "getDismissedUpsells" }) + }) + + it("should handle dismissed upsells message from extension", async () => { + const TestComponent = () => { + const { dismissedUpsells, isLoading } = useDismissedUpsells() + return ( +
+
{isLoading.toString()}
+
{dismissedUpsells.length}
+ {dismissedUpsells.map((id) => ( +
+ {id} +
+ ))} +
+ ) + } + + render( + + + , + ) + + // Initially loading + expect(screen.getByTestId("loading")).toHaveTextContent("true") + expect(screen.getByTestId("count")).toHaveTextContent("0") + + // Simulate message from extension + act(() => { + messageHandler?.({ + data: { + type: "dismissedUpsells", + list: ["upsell-1", "upsell-2"], + }, + } as MessageEvent) + }) + + await waitFor(() => { + expect(screen.getByTestId("loading")).toHaveTextContent("false") + expect(screen.getByTestId("count")).toHaveTextContent("2") + expect(screen.getByTestId("dismissed-upsell-1")).toHaveTextContent("upsell-1") + expect(screen.getByTestId("dismissed-upsell-2")).toHaveTextContent("upsell-2") + }) + }) + + it("should handle empty dismissed upsells list", async () => { + const TestComponent = () => { + const { isLoading } = useDismissedUpsells() + return
{isLoading.toString()}
+ } + + render( + + + , + ) + + act(() => { + messageHandler?.({ + data: { + type: "dismissedUpsells", + list: [], + }, + } as MessageEvent) + }) + + await waitFor(() => { + expect(screen.getByTestId("loading")).toHaveTextContent("false") + }) + }) + + it("should ignore invalid messages", () => { + const TestComponent = () => { + const { isLoading } = useDismissedUpsells() + return
{isLoading.toString()}
+ } + + render( + + + , + ) + + // Invalid message type + act(() => { + messageHandler?.({ + data: { + type: "otherType", + list: ["upsell-1"], + }, + } as MessageEvent) + }) + + // Still loading + expect(screen.getByTestId("loading")).toHaveTextContent("true") + + // Invalid list format + act(() => { + messageHandler?.({ + data: { + type: "dismissedUpsells", + list: "not-an-array", + }, + } as MessageEvent) + }) + + // Still loading + expect(screen.getByTestId("loading")).toHaveTextContent("true") + }) + }) + + describe("useDismissedUpsells", () => { + it("should throw error when used outside provider", () => { + // Suppress console.error for this test + const spy = vi.spyOn(console, "error").mockImplementation(() => {}) + + const { result } = renderHook(() => { + try { + return useDismissedUpsells() + } catch (error: any) { + return { error } + } + }) + + expect((result.current as any).error).toBeDefined() + expect((result.current as any).error.message).toBe( + "useDismissedUpsells must be used within a DismissedUpsellsProvider", + ) + + spy.mockRestore() + }) + + it("should check if upsell is visible", async () => { + const TestComponent = () => { + const { isUpsellVisible } = useDismissedUpsells() + return ( +
+
{isUpsellVisible("upsell-1").toString()}
+
{isUpsellVisible("upsell-2").toString()}
+
{isUpsellVisible("upsell-3").toString()}
+
+ ) + } + + render( + + + , + ) + + // Send dismissed list + act(() => { + messageHandler?.({ + data: { + type: "dismissedUpsells", + list: ["upsell-1", "upsell-3"], + }, + } as MessageEvent) + }) + + await waitFor(() => { + expect(screen.getByTestId("visible-1")).toHaveTextContent("false") + expect(screen.getByTestId("visible-2")).toHaveTextContent("true") + expect(screen.getByTestId("visible-3")).toHaveTextContent("false") + }) + }) + + it("should dismiss an upsell", async () => { + const TestComponent = () => { + const { dismissUpsell, isUpsellVisible } = useDismissedUpsells() + return ( +
+
{isUpsellVisible("test-upsell").toString()}
+ +
+ ) + } + + const { getByText } = render( + + + , + ) + + // Initially visible + act(() => { + messageHandler?.({ + data: { + type: "dismissedUpsells", + list: [], + }, + } as MessageEvent) + }) + + await waitFor(() => { + expect(screen.getByTestId("visible")).toHaveTextContent("true") + }) + + // Dismiss the upsell + act(() => { + getByText("Dismiss").click() + }) + + // Should send message to extension + expect(vscode.postMessage).toHaveBeenCalledWith({ + type: "dismissUpsell", + upsellId: "test-upsell", + }) + + // Should update local state optimistically + await waitFor(() => { + expect(screen.getByTestId("visible")).toHaveTextContent("false") + }) + }) + + it("should not dismiss already dismissed upsell", async () => { + const TestComponent = () => { + const { dismissUpsell } = useDismissedUpsells() + return + } + + const { getByText } = render( + + + , + ) + + // Set already dismissed + act(() => { + messageHandler?.({ + data: { + type: "dismissedUpsells", + list: ["already-dismissed"], + }, + } as MessageEvent) + }) + + // Clear previous calls + vi.clearAllMocks() + + // Try to dismiss again + act(() => { + getByText("Dismiss").click() + }) + + // Should not send message + expect(vscode.postMessage).not.toHaveBeenCalled() + }) + + it("should refresh dismissed upsells", () => { + const TestComponent = () => { + const { refreshDismissedUpsells } = useDismissedUpsells() + return + } + + const { getByText } = render( + + + , + ) + + // Clear initial call + vi.clearAllMocks() + + act(() => { + getByText("Refresh").click() + }) + + expect(vscode.postMessage).toHaveBeenCalledWith({ type: "getDismissedUpsells" }) + }) + }) + + describe("useIsUpsellVisible", () => { + it("should return visibility status for specific upsell", async () => { + const TestComponent = ({ upsellId }: { upsellId: string }) => { + const isVisible = useIsUpsellVisible(upsellId) + return
{isVisible.toString()}
+ } + + render( + + + , + ) + + // Send dismissed list without the test upsell + act(() => { + messageHandler?.({ + data: { + type: "dismissedUpsells", + list: ["other-upsell"], + }, + } as MessageEvent) + }) + + await waitFor(() => { + expect(screen.getByTestId("visible")).toHaveTextContent("true") + }) + + // Send dismissed list with the test upsell + act(() => { + messageHandler?.({ + data: { + type: "dismissedUpsells", + list: ["test-upsell", "other-upsell"], + }, + } as MessageEvent) + }) + + await waitFor(() => { + expect(screen.getByTestId("visible")).toHaveTextContent("false") + }) + }) + + it("should work with UPSELL_IDS constants", async () => { + const TestComponent = () => { + const isVisible = useIsUpsellVisible(UPSELL_IDS.TASK_LIST) + return
{isVisible.toString()}
+ } + + render( + + + , + ) + + // Send dismissed list with the constant + act(() => { + messageHandler?.({ + data: { + type: "dismissedUpsells", + list: [UPSELL_IDS.TASK_LIST], + }, + } as MessageEvent) + }) + + await waitFor(() => { + expect(screen.getByTestId("visible")).toHaveTextContent("false") + }) + }) + }) +}) diff --git a/webview-ui/src/hooks/__tests__/useUpsellVisibility.spec.tsx b/webview-ui/src/hooks/__tests__/useUpsellVisibility.spec.tsx new file mode 100644 index 0000000000..1328acdfe8 --- /dev/null +++ b/webview-ui/src/hooks/__tests__/useUpsellVisibility.spec.tsx @@ -0,0 +1,299 @@ +import { renderHook } from "@testing-library/react" +import React, { ReactNode } from "react" +import { DismissedUpsellsProvider } from "@src/context/DismissedUpsellsContext" +import { + useUpsellVisibility, + useMultipleUpsellVisibility, + useAnyUpsellVisible, + useAllUpsellsVisible, + useUpsellManager, +} from "../useUpsellVisibility" +import { UPSELL_IDS } from "@src/constants/upsellIds" +import { act } from "@testing-library/react" + +// Mock vscode +vi.mock("@src/utils/vscode", () => ({ + vscode: { + postMessage: vi.fn(), + }, +})) + +describe("useUpsellVisibility hooks", () => { + let messageHandler: ((event: MessageEvent) => void) | null = null + + const wrapper = ({ children }: { children: ReactNode }) => ( + {children} + ) + + beforeEach(() => { + vi.clearAllMocks() + + // Capture the message event handler + window.addEventListener = vi.fn((event, handler) => { + if (event === "message") { + messageHandler = handler as (event: MessageEvent) => void + } + }) + + window.removeEventListener = vi.fn() + }) + + afterEach(() => { + messageHandler = null + }) + + const sendDismissedList = (list: string[]) => { + act(() => { + messageHandler?.({ + data: { + type: "dismissedUpsells", + list, + }, + } as MessageEvent) + }) + } + + describe("useUpsellVisibility", () => { + it("should return true when upsell is not dismissed", () => { + const { result } = renderHook(() => useUpsellVisibility("test-upsell"), { wrapper }) + + sendDismissedList([]) + + expect(result.current).toBe(true) + }) + + it("should return false when upsell is dismissed", () => { + const { result } = renderHook(() => useUpsellVisibility("test-upsell"), { wrapper }) + + sendDismissedList(["test-upsell"]) + + expect(result.current).toBe(false) + }) + + it("should work with UPSELL_IDS constants", () => { + const { result } = renderHook(() => useUpsellVisibility(UPSELL_IDS.TASK_LIST), { wrapper }) + + sendDismissedList([UPSELL_IDS.TASK_LIST]) + + expect(result.current).toBe(false) + }) + + it("should update when dismissed list changes", () => { + const { result } = renderHook(() => useUpsellVisibility("dynamic-upsell"), { wrapper }) + + sendDismissedList([]) + expect(result.current).toBe(true) + + sendDismissedList(["dynamic-upsell"]) + expect(result.current).toBe(false) + + sendDismissedList([]) + expect(result.current).toBe(true) + }) + }) + + describe("useMultipleUpsellVisibility", () => { + it("should return visibility status for multiple upsells", () => { + const upsellIds = ["upsell-1", "upsell-2", "upsell-3"] + const { result } = renderHook(() => useMultipleUpsellVisibility(upsellIds), { wrapper }) + + sendDismissedList(["upsell-1", "upsell-3"]) + + expect(result.current).toEqual({ + "upsell-1": false, + "upsell-2": true, + "upsell-3": false, + }) + }) + + it("should handle empty array", () => { + const { result } = renderHook(() => useMultipleUpsellVisibility([]), { wrapper }) + + sendDismissedList(["some-upsell"]) + + expect(result.current).toEqual({}) + }) + + it("should update when dismissed list changes", () => { + const upsellIds = ["upsell-a", "upsell-b"] + const { result } = renderHook(() => useMultipleUpsellVisibility(upsellIds), { wrapper }) + + sendDismissedList([]) + expect(result.current).toEqual({ + "upsell-a": true, + "upsell-b": true, + }) + + sendDismissedList(["upsell-a"]) + expect(result.current).toEqual({ + "upsell-a": false, + "upsell-b": true, + }) + }) + }) + + describe("useAnyUpsellVisible", () => { + it("should return true if at least one upsell is visible", () => { + const upsellIds = ["upsell-1", "upsell-2", "upsell-3"] + const { result } = renderHook(() => useAnyUpsellVisible(upsellIds), { wrapper }) + + sendDismissedList(["upsell-1", "upsell-3"]) + + expect(result.current).toBe(true) + }) + + it("should return false if all upsells are dismissed", () => { + const upsellIds = ["upsell-1", "upsell-2", "upsell-3"] + const { result } = renderHook(() => useAnyUpsellVisible(upsellIds), { wrapper }) + + sendDismissedList(["upsell-1", "upsell-2", "upsell-3"]) + + expect(result.current).toBe(false) + }) + + it("should return true if no upsells are dismissed", () => { + const upsellIds = ["upsell-1", "upsell-2"] + const { result } = renderHook(() => useAnyUpsellVisible(upsellIds), { wrapper }) + + sendDismissedList([]) + + expect(result.current).toBe(true) + }) + + it("should handle empty array", () => { + const { result } = renderHook(() => useAnyUpsellVisible([]), { wrapper }) + + sendDismissedList(["some-upsell"]) + + expect(result.current).toBe(false) + }) + }) + + describe("useAllUpsellsVisible", () => { + it("should return true if all upsells are visible", () => { + const upsellIds = ["upsell-1", "upsell-2", "upsell-3"] + const { result } = renderHook(() => useAllUpsellsVisible(upsellIds), { wrapper }) + + sendDismissedList(["other-upsell"]) + + expect(result.current).toBe(true) + }) + + it("should return false if at least one upsell is dismissed", () => { + const upsellIds = ["upsell-1", "upsell-2", "upsell-3"] + const { result } = renderHook(() => useAllUpsellsVisible(upsellIds), { wrapper }) + + sendDismissedList(["upsell-2"]) + + expect(result.current).toBe(false) + }) + + it("should return false if all upsells are dismissed", () => { + const upsellIds = ["upsell-1", "upsell-2"] + const { result } = renderHook(() => useAllUpsellsVisible(upsellIds), { wrapper }) + + sendDismissedList(["upsell-1", "upsell-2"]) + + expect(result.current).toBe(false) + }) + + it("should handle empty array", () => { + const { result } = renderHook(() => useAllUpsellsVisible([]), { wrapper }) + + sendDismissedList(["some-upsell"]) + + expect(result.current).toBe(true) + }) + }) + + describe("useUpsellManager", () => { + it("should provide dismissUpsell function", () => { + const { result } = renderHook(() => useUpsellManager(), { wrapper }) + + sendDismissedList([]) + + act(() => { + result.current.dismissUpsell("test-upsell") + }) + + expect(result.current.dismissedUpsells).toContain("test-upsell") + }) + + it("should provide dismissMultiple function", () => { + const { result } = renderHook(() => useUpsellManager(), { wrapper }) + + sendDismissedList([]) + + act(() => { + result.current.dismissMultiple(["upsell-1", "upsell-2", "upsell-3"]) + }) + + expect(result.current.dismissedUpsells).toContain("upsell-1") + expect(result.current.dismissedUpsells).toContain("upsell-2") + expect(result.current.dismissedUpsells).toContain("upsell-3") + }) + + it("should provide getDismissedCount function", () => { + const { result } = renderHook(() => useUpsellManager(), { wrapper }) + + sendDismissedList(["upsell-1", "upsell-2"]) + + expect(result.current.getDismissedCount()).toBe(2) + }) + + it("should provide dismissedUpsells array", () => { + const { result } = renderHook(() => useUpsellManager(), { wrapper }) + + sendDismissedList(["upsell-a", "upsell-b", "upsell-c"]) + + expect(result.current.dismissedUpsells).toEqual(["upsell-a", "upsell-b", "upsell-c"]) + }) + + it("should provide isLoading state", () => { + const { result } = renderHook(() => useUpsellManager(), { wrapper }) + + expect(result.current.isLoading).toBe(true) + + sendDismissedList([]) + + expect(result.current.isLoading).toBe(false) + }) + + it("should log warning for clearAllDismissed", () => { + const consoleSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + const { result } = renderHook(() => useUpsellManager(), { wrapper }) + + act(() => { + result.current.clearAllDismissed() + }) + + expect(consoleSpy).toHaveBeenCalledWith( + "clearAllDismissed is not yet implemented. It requires extension-side support.", + ) + + consoleSpy.mockRestore() + }) + + it("should not dismiss same upsell multiple times", () => { + const { result } = renderHook(() => useUpsellManager(), { wrapper }) + + sendDismissedList([]) + + // Dismiss once + act(() => { + result.current.dismissUpsell("test-upsell") + }) + + // Wait for the state to update + expect(result.current.dismissedUpsells).toContain("test-upsell") + + // Try to dismiss again (should not add duplicate) + act(() => { + result.current.dismissUpsell("test-upsell") + }) + + const count = result.current.dismissedUpsells.filter((id) => id === "test-upsell").length + expect(count).toBe(1) + }) + }) +}) diff --git a/webview-ui/src/hooks/useUpsellVisibility.ts b/webview-ui/src/hooks/useUpsellVisibility.ts new file mode 100644 index 0000000000..75ae76d77f --- /dev/null +++ b/webview-ui/src/hooks/useUpsellVisibility.ts @@ -0,0 +1,95 @@ +import { useCallback, useMemo } from "react" +import { useDismissedUpsells, useIsUpsellVisible } from "@src/context/DismissedUpsellsContext" +import { UpsellId, UPSELL_IDS, isValidUpsellId } from "@src/constants/upsellIds" + +// Re-export upsell constants and types for convenience +export { UPSELL_IDS, isValidUpsellId } +export type { UpsellId } + +/** + * Hook to check visibility of a single upsell + * @param upsellId - The ID of the upsell to check + * @returns Whether the upsell is visible (not dismissed) + */ +export function useUpsellVisibility(upsellId: UpsellId | string): boolean { + return useIsUpsellVisible(upsellId) +} + +/** + * Hook to check visibility of multiple upsells + * @param upsellIds - Array of upsell IDs to check + * @returns Object with visibility status for each upsell ID + */ +export function useMultipleUpsellVisibility(upsellIds: (UpsellId | string)[]): Record { + const { isUpsellVisible } = useDismissedUpsells() + + return useMemo(() => { + const result: Record = {} + + for (const id of upsellIds) { + result[id] = isUpsellVisible(id) + } + + return result + }, [upsellIds, isUpsellVisible]) +} + +/** + * Hook to check if any of the provided upsells are visible + * @param upsellIds - Array of upsell IDs to check + * @returns True if at least one upsell is visible + */ +export function useAnyUpsellVisible(upsellIds: (UpsellId | string)[]): boolean { + const { isUpsellVisible } = useDismissedUpsells() + + return useMemo(() => { + return upsellIds.some((id) => isUpsellVisible(id)) + }, [upsellIds, isUpsellVisible]) +} + +/** + * Hook to check if all of the provided upsells are visible + * @param upsellIds - Array of upsell IDs to check + * @returns True if all upsells are visible + */ +export function useAllUpsellsVisible(upsellIds: (UpsellId | string)[]): boolean { + const { isUpsellVisible } = useDismissedUpsells() + + return useMemo(() => { + return upsellIds.every((id) => isUpsellVisible(id)) + }, [upsellIds, isUpsellVisible]) +} + +/** + * Hook that provides upsell management functions + * @returns Object with upsell management functions + */ +export function useUpsellManager() { + const { dismissUpsell, refreshDismissedUpsells, dismissedUpsells, isLoading } = useDismissedUpsells() + + const dismissMultiple = useCallback( + (upsellIds: (UpsellId | string)[]) => { + upsellIds.forEach((id) => dismissUpsell(id)) + }, + [dismissUpsell], + ) + + const getDismissedCount = useCallback(() => { + return dismissedUpsells.length + }, [dismissedUpsells]) + + const clearAllDismissed = useCallback(() => { + // This would require a new message type to the extension + console.warn("clearAllDismissed is not yet implemented. It requires extension-side support.") + }, []) + + return { + dismissUpsell, + dismissMultiple, + refreshDismissedUpsells, + getDismissedCount, + clearAllDismissed, + dismissedUpsells, + isLoading, + } +} From f10bbef3333bc173720c3ef4ea6a4b2d2455d333 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Mon, 15 Sep 2025 12:51:24 +0000 Subject: [PATCH 2/2] fix: address PR review feedback - Change isLoading default to false in DismissedUpsellsContext to prevent flash of upsells - Fix import path inconsistency in ChatView.tsx (use @src/ instead of @/) --- webview-ui/src/components/chat/ChatView.tsx | 2 +- webview-ui/src/context/DismissedUpsellsContext.tsx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/webview-ui/src/components/chat/ChatView.tsx b/webview-ui/src/components/chat/ChatView.tsx index c647750516..d6d22f3b53 100644 --- a/webview-ui/src/components/chat/ChatView.tsx +++ b/webview-ui/src/components/chat/ChatView.tsx @@ -59,7 +59,7 @@ import { Cloud } from "lucide-react" import { CloudUpsellDialog } from "@src/components/cloud/CloudUpsellDialog" import DismissibleUpsell from "../common/DismissibleUpsell" import { useCloudUpsell } from "@src/hooks/useCloudUpsell" -import { useUpsellVisibility, UPSELL_IDS } from "@/hooks/useUpsellVisibility" +import { useUpsellVisibility, UPSELL_IDS } from "@src/hooks/useUpsellVisibility" export interface ChatViewProps { isHidden: boolean diff --git a/webview-ui/src/context/DismissedUpsellsContext.tsx b/webview-ui/src/context/DismissedUpsellsContext.tsx index 51802bc6a8..4924bc31a7 100644 --- a/webview-ui/src/context/DismissedUpsellsContext.tsx +++ b/webview-ui/src/context/DismissedUpsellsContext.tsx @@ -23,7 +23,7 @@ interface DismissedUpsellsProviderProps { export const DismissedUpsellsProvider: React.FC = ({ children }) => { const [dismissedUpsells, setDismissedUpsells] = useState([]) - const [isLoading, setIsLoading] = useState(true) + const [isLoading, setIsLoading] = useState(false) // Request dismissed upsells from extension const refreshDismissedUpsells = useCallback(() => {