Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
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 packages/types/src/global-settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export const globalSettingsSchema = z.object({
lastShownAnnouncementId: z.string().optional(),
customInstructions: z.string().optional(),
taskHistory: z.array(historyItemSchema).optional(),
dismissedUpsells: z.array(z.string()).optional(),

// Image generation settings (experimental) - flattened for simplicity
openRouterImageApiKey: z.string().optional(),
Expand Down
28 changes: 28 additions & 0 deletions src/core/webview/webviewMessageHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2998,5 +2998,33 @@ export const webviewMessageHandler = async (

break
}
case "dismissUpsell": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missing error handling. What happens if updateGlobalState fails? Consider wrapping in try-catch and logging any errors.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's OK to fail silently in this case

if (message.upsellId) {
// Get current list of dismissed upsells
const dismissedUpsells = getGlobalState("dismissedUpsells") || []

// Add the new upsell ID if not already present
if (!dismissedUpsells.includes(message.upsellId)) {
const updatedList = [...dismissedUpsells, message.upsellId]
await updateGlobalState("dismissedUpsells", updatedList)
}

// Send updated list back to webview
await provider.postMessageToWebview({
type: "dismissedUpsells",
list: [...dismissedUpsells, message.upsellId],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inefficient array operations - you're spreading the dismissedUpsells array twice unnecessarily. The updatedList already contains all items, so just use: list: updatedList

})
}
break
}
case "getDismissedUpsells": {
// Send the current list of dismissed upsells to the webview
const dismissedUpsells = getGlobalState("dismissedUpsells") || []
await provider.postMessageToWebview({
type: "dismissedUpsells",
list: dismissedUpsells,
})
break
}
}
}
3 changes: 3 additions & 0 deletions src/shared/ExtensionMessage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ export interface ExtensionMessage {
| "showEditMessageDialog"
| "commands"
| "insertTextIntoTextarea"
| "dismissedUpsells"
text?: string
payload?: any // Add a generic payload for now, can refine later
action?:
Expand Down Expand Up @@ -199,6 +200,7 @@ export interface ExtensionMessage {
context?: string
commands?: Command[]
queuedMessages?: QueuedMessage[]
list?: string[] // For dismissedUpsells
}

export type ExtensionState = Pick<
Expand All @@ -209,6 +211,7 @@ export type ExtensionState = Pick<
// | "lastShownAnnouncementId"
| "customInstructions"
// | "taskHistory" // Optional in GlobalSettings, required here.
| "dismissedUpsells"
| "autoApprovalEnabled"
| "alwaysAllowReadOnly"
| "alwaysAllowReadOnlyOutsideWorkspace"
Expand Down
4 changes: 4 additions & 0 deletions src/shared/WebviewMessage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,8 @@ export interface WebviewMessage {
| "queueMessage"
| "removeQueuedMessage"
| "editQueuedMessage"
| "dismissUpsell"
| "getDismissedUpsells"
text?: string
editedMessageContent?: string
tab?: "settings" | "history" | "mcp" | "modes" | "chat" | "marketplace" | "cloud"
Expand Down Expand Up @@ -267,6 +269,8 @@ export interface WebviewMessage {
visibility?: ShareVisibility // For share visibility
hasContent?: boolean // For checkRulesDirectoryResult
checkOnly?: boolean // For deleteCustomMode check
upsellId?: string // For dismissUpsell
list?: string[] // For dismissedUpsells response
codeIndexSettings?: {
// Global state settings
codebaseIndexEnabled: boolean
Expand Down
144 changes: 144 additions & 0 deletions webview-ui/src/components/common/DismissibleUpsell.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
import { memo, ReactNode, useEffect, useState } from "react"
import styled from "styled-components"

import { vscode } from "@src/utils/vscode"

interface DismissibleUpsellProps {
/** Required unique identifier for this upsell */
className: string
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using a dedicated prop (e.g. 'upsellId') for the unique identifier instead of overloading the 'className' prop, which is typically used for styling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using className as a unique identifier is unconventional and could be confusing. The className prop is typically used for CSS styling. Consider adding a dedicated id prop instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is wrong. There should be two different attributes - className and name.

/** Content to display inside the upsell */
children: ReactNode
/** Visual variant of the upsell */
variant?: "banner" | "default"
/** Optional callback when upsell is dismissed */
onDismiss?: () => void
}

const UpsellContainer = styled.div<{ $variant: "banner" | "default" }>`
position: relative;
padding: 12px 40px 12px 16px;
border-radius: 6px;
margin-bottom: 8px;
display: flex;
align-items: center;
${(props) =>
props.$variant === "banner"
? `
background-color: var(--vscode-button-background);
color: var(--vscode-button-foreground);
`
: `
background-color: var(--vscode-notifications-background);
color: var(--vscode-notifications-foreground);
border: 1px solid var(--vscode-notifications-border);
`}
`

const DismissButton = styled.button<{ $variant: "banner" | "default" }>`
position: absolute;
top: 50%;
right: 12px;
transform: translateY(-50%);
background: none;
border: none;
cursor: pointer;
padding: 4px;
display: flex;
align-items: center;
justify-content: center;
border-radius: 4px;
transition: background-color 0.2s;
${(props) =>
props.$variant === "banner"
? `
color: var(--vscode-button-foreground);
&:hover {
background-color: rgba(255, 255, 255, 0.1);
}
`
: `
color: var(--vscode-notifications-foreground);
&:hover {
background-color: var(--vscode-toolbar-hoverBackground);
}
`}
&:focus {
outline: 1px solid var(--vscode-focusBorder);
outline-offset: 1px;
}
`

const DismissIcon = () => (
<svg width="16" height="16" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg" aria-hidden="true">
<path
fillRule="evenodd"
clipRule="evenodd"
d="M8 8.707l3.646 3.647.708-.707L8.707 8l3.647-3.646-.707-.708L8 7.293 4.354 3.646l-.707.708L7.293 8l-3.647 3.646.708.707L8 8.707z"
fill="currentColor"
/>
</svg>
)

const DismissibleUpsell = memo(({ className, children, variant = "banner", onDismiss }: DismissibleUpsellProps) => {
const [isVisible, setIsVisible] = useState(true)

useEffect(() => {
// Request the current list of dismissed upsells from the extension
vscode.postMessage({ type: "getDismissedUpsells" })

// Listen for the response
const handleMessage = (event: MessageEvent) => {
const message = event.data
if (message.type === "dismissedUpsells" && Array.isArray(message.list)) {
// Check if this upsell has been dismissed
if (message.list.includes(className)) {
setIsVisible(false)
}
}
}

window.addEventListener("message", handleMessage)
return () => window.removeEventListener("message", handleMessage)
}, [className])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Potential memory leak here. The event listener cleanup in the useEffect return function might not execute if the component unmounts while waiting for the initial response. Consider using a mounted flag to prevent state updates after unmount.


const handleDismiss = () => {
// Hide the upsell immediately
setIsVisible(false)

// Notify the extension to persist the dismissal
vscode.postMessage({
type: "dismissUpsell",
upsellId: className,
})

// Call the optional callback
onDismiss?.()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Race condition: If the component unmounts immediately after clicking dismiss but before the message is sent, the dismissal won't be persisted. Consider ensuring the message is sent before hiding the component.


// Don't render if not visible
if (!isVisible) {
return null
}

return (
<UpsellContainer $variant={variant} className={className}>
{children}
<DismissButton
$variant={variant}
onClick={handleDismiss}
aria-label="Dismiss"
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace hardcoded accessibility strings with translation keys using the i18n function to ensure localization.

Suggested change
aria-label="Dismiss"
aria-label={i18n('dismiss')}

This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.

title="Dismiss and don't show again">
<DismissIcon />
</DismissButton>
</UpsellContainer>
)
})

DismissibleUpsell.displayName = "DismissibleUpsell"

export default DismissibleUpsell
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
import { render, screen, fireEvent, waitFor } from "@testing-library/react"
import { describe, it, expect, vi, beforeEach } from "vitest"
import DismissibleUpsell from "../DismissibleUpsell"

// Mock the vscode API
const mockPostMessage = vi.fn()
vi.mock("@src/utils/vscode", () => ({
vscode: {
postMessage: (message: any) => mockPostMessage(message),
},
}))

describe("DismissibleUpsell", () => {
beforeEach(() => {
mockPostMessage.mockClear()
})

it("renders children content", () => {
render(
<DismissibleUpsell className="test-upsell">
<div>Test content</div>
</DismissibleUpsell>,
)

expect(screen.getByText("Test content")).toBeInTheDocument()
})

it("applies the correct variant styles", () => {
const { container, rerender } = render(
<DismissibleUpsell className="test-upsell" variant="banner">
<div>Banner content</div>
</DismissibleUpsell>,
)

// Check banner variant has correct background color style
const bannerContainer = container.firstChild
expect(bannerContainer).toHaveStyle({
backgroundColor: "var(--vscode-button-background)",
color: "var(--vscode-button-foreground)",
})

// Re-render with default variant
rerender(
<DismissibleUpsell className="test-upsell" variant="default">
<div>Default content</div>
</DismissibleUpsell>,
)

const defaultContainer = container.firstChild
expect(defaultContainer).toHaveStyle({
backgroundColor: "var(--vscode-notifications-background)",
color: "var(--vscode-notifications-foreground)",
})
})

it("requests dismissed upsells list on mount", () => {
render(
<DismissibleUpsell className="test-upsell">
<div>Test content</div>
</DismissibleUpsell>,
)

expect(mockPostMessage).toHaveBeenCalledWith({
type: "getDismissedUpsells",
})
})

it("hides the upsell when dismiss button is clicked", async () => {
const onDismiss = vi.fn()
const { container } = render(
<DismissibleUpsell className="test-upsell" onDismiss={onDismiss}>
<div>Test content</div>
</DismissibleUpsell>,
)

// Find and click the dismiss button
const dismissButton = screen.getByRole("button", { name: /dismiss/i })
fireEvent.click(dismissButton)

// Check that the component is no longer visible
await waitFor(() => {
expect(container.firstChild).toBeNull()
})

// Check that the dismiss message was sent
expect(mockPostMessage).toHaveBeenCalledWith({
type: "dismissUpsell",
upsellId: "test-upsell",
})

// Check that the callback was called
expect(onDismiss).toHaveBeenCalled()
})

it("hides the upsell if it's in the dismissed list", async () => {
const { container } = render(
<DismissibleUpsell className="test-upsell">
<div>Test content</div>
</DismissibleUpsell>,
)

// 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)

// Check that the component is no longer visible
await waitFor(() => {
expect(container.firstChild).toBeNull()
})
})

it("remains visible if not in the dismissed list", async () => {
render(
<DismissibleUpsell className="test-upsell">
<div>Test content</div>
</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)

// Check that the component is still visible
await waitFor(() => {
expect(screen.getByText("Test content")).toBeInTheDocument()
})
})

it("applies the className prop to the container", () => {
const { container } = render(
<DismissibleUpsell className="custom-class">
<div>Test content</div>
</DismissibleUpsell>,
)

expect(container.firstChild).toHaveClass("custom-class")
})

it("dismiss button has proper accessibility attributes", () => {
render(
<DismissibleUpsell className="test-upsell">
<div>Test content</div>
</DismissibleUpsell>,
)

const dismissButton = screen.getByRole("button", { name: /dismiss/i })
expect(dismissButton).toHaveAttribute("aria-label", "Dismiss")
expect(dismissButton).toHaveAttribute("title", "Dismiss and don't show again")
})
})
Loading