Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import {addSuccessMessage} from 'sentry/actionCreators/indicator';
import {CommandPaletteProvider} from 'sentry/components/commandPalette/ui/cmdk';
import {CMDKAction, CMDKGroup} from 'sentry/components/commandPalette/ui/cmdk';
import {CMDKAction} from 'sentry/components/commandPalette/ui/cmdk';
import type {CMDKActionData} from 'sentry/components/commandPalette/ui/cmdk';
import type {CollectionTreeNode} from 'sentry/components/commandPalette/ui/collection';
import {CommandPalette} from 'sentry/components/commandPalette/ui/commandPalette';
Expand All @@ -15,7 +15,7 @@
const handleAction = useCallback(
(action: CollectionTreeNode<CMDKActionData>) => {
if ('to' in action) {
navigate(normalizeUrl(String(action.to)));

Check failure on line 18 in static/app/components/commandPalette/__stories__/components.tsx

View workflow job for this annotation

GitHub Actions / pre-commit lint

'action.to' may use Object's default stringification format ('[object Object]') when stringified

Check failure on line 18 in static/app/components/commandPalette/__stories__/components.tsx

View workflow job for this annotation

GitHub Actions / eslint

'action.to' may use Object's default stringification format ('[object Object]') when stringified
} else if ('onAction' in action) {
action.onAction();
}
Expand All @@ -30,14 +30,14 @@
display={{label: 'Execute an action'}}
onAction={() => addSuccessMessage('Action executed')}
/>
<CMDKGroup display={{label: 'Parent action'}}>
<CMDKAction display={{label: 'Parent action'}}>
<CMDKAction
display={{label: 'Child action'}}
onAction={() => addSuccessMessage('Child action executed')}
/>
</CMDKGroup>
</CMDKAction>
<CommandPalette onAction={handleAction}>
<CMDKGroup display={{label: 'Issues List'}}>
<CMDKAction display={{label: 'Issues List'}}>
<CMDKAction
display={{label: 'Select all'}}
onAction={() => addSuccessMessage('Select all')}
Expand All @@ -46,7 +46,7 @@
display={{label: 'Deselect all'}}
onAction={() => addSuccessMessage('Deselect all')}
/>
</CMDKGroup>
</CMDKAction>
</CommandPalette>
</CommandPaletteProvider>
);
Expand Down
51 changes: 30 additions & 21 deletions static/app/components/commandPalette/ui/cmdk.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ interface CMDKActionDataBase {
}

interface CMDKActionDataTo extends CMDKActionDataBase {
to: string;
to: LocationDescriptor;
}

interface CMDKActionDataOnAction extends CMDKActionDataBase {
Expand All @@ -50,7 +50,7 @@ export const CMDKCollection = makeCollection<CMDKActionData>();

/**
* Root provider for the command palette. Wrap the component tree that
* contains CMDKGroup/CMDKAction registrations and the CommandPalette UI.
* contains CMDKAction registrations and the CommandPalette UI.
*/
export function CommandPaletteProvider({children}: {children: React.ReactNode}) {
return (
Expand All @@ -62,25 +62,39 @@ export function CommandPaletteProvider({children}: {children: React.ReactNode})
);
}

interface CMDKGroupProps {
interface CMDKActionProps {
display: DisplayProps;
children?: React.ReactNode | ((data: CommandPaletteAsyncResult[]) => React.ReactNode);
keywords?: string[];
onAction?: () => void;
resource?: (query: string) => CMDKQueryOptions;
to?: LocationDescriptor;
}

type CMDKActionProps =
| {display: DisplayProps; to: LocationDescriptor; keywords?: string[]}
| {display: DisplayProps; onAction: () => void; keywords?: string[]};

/**
* Registers a node in the collection and propagates its key to children via
* GroupContext. When a `resource` prop is provided, fetches data using the
* current query and passes results to a render-prop children function.
* Registers a node in the collection. A node becomes a group when it has
* children — they register under this node as their parent. Provide `to` for
* navigation, `onAction` for a callback, or `resource` with a render-prop
* children function to fetch and populate async results.
*/
export function CMDKGroup({display, keywords, resource, children}: CMDKGroupProps) {
export function CMDKAction({
display,
keywords,
children,
to,
onAction,
resource,
}: CMDKActionProps) {
const ref = CommandPaletteSlot.useSlotOutletRef();
const key = CMDKCollection.useRegisterNode({display, keywords, resource, ref});

const nodeData: CMDKActionData =
to === undefined
? onAction === undefined
? {display, keywords, ref, resource}
: {display, keywords, ref, onAction}
Comment on lines 88 to +94
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug: The CMDKAction component silently ignores the onAction prop if the to prop is also provided, due to a loss of type-level mutual exclusivity.
Severity: MEDIUM

Suggested Fix

Restore the type-level safety by using a discriminated union for the CMDKActionProps type. This will enforce that either to or onAction can be provided, but not both, preventing this issue at compile time. This was the behavior in the previous version of the component.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: static/app/components/commandPalette/ui/cmdk.tsx#L88-L94

Potential issue: The `CMDKAction` component's props interface was changed from a
discriminated union to a single interface with optional `to` and `onAction` props. This
removes the compile-time guarantee that only one can be provided. The component's
implementation prioritizes the `to` prop for navigation. If a developer provides both
`to` and `onAction` on the same component, the `onAction` callback will be silently
ignored, and only the navigation will occur. This is a type safety regression that could
lead to unexpected behavior where a developer's intended action does not execute.

Did we get this right? 👍 / 👎 to inform future reviews.

: {display, keywords, ref, to};

const key = CMDKCollection.useRegisterNode(nodeData);
const {query} = useCommandPaletteState();

const resourceOptions = resource
Expand All @@ -91,6 +105,10 @@ export function CMDKGroup({display, keywords, resource, children}: CMDKGroupProp
enabled: !!resource && (resourceOptions.enabled ?? true),
});

if (!children) {
return null;
}

const resolvedChildren =
typeof children === 'function' ? (data ? children(data) : null) : children;

Expand All @@ -100,12 +118,3 @@ export function CMDKGroup({display, keywords, resource, children}: CMDKGroupProp
</CMDKCollection.Context.Provider>
);
}

/**
* Registers a leaf action node in the collection.
*/
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Leaf actions unnecessarily subscribe to query state

Medium Severity

The merged CMDKAction unconditionally calls useCommandPaletteState() and useQuery() for every instance, including leaf actions that have no resource and no children. Previously, leaf CMDKAction was lightweight (register + return null). Now every leaf subscribes to the query context, causing all leaf actions to re-render on every keystroke in the palette. With dozens of static leaves plus dynamic ones (projects, settings), this is a meaningful regression in a typing-heavy interaction.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit c85059a. Configure here.

export function CMDKAction(props: CMDKActionProps) {
const ref = CommandPaletteSlot.useSlotOutletRef();
CMDKCollection.useRegisterNode({...props, ref});
return null;
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
import * as modalActions from 'sentry/actionCreators/modal';
import type {CommandPaletteAction} from 'sentry/components/commandPalette/types';
import {CommandPaletteProvider} from 'sentry/components/commandPalette/ui/cmdk';
import {CMDKAction, CMDKGroup} from 'sentry/components/commandPalette/ui/cmdk';
import {CMDKAction} from 'sentry/components/commandPalette/ui/cmdk';
import type {CMDKActionData} from 'sentry/components/commandPalette/ui/cmdk';
import type {CollectionTreeNode} from 'sentry/components/commandPalette/ui/collection';
import {CommandPalette} from 'sentry/components/commandPalette/ui/commandPalette';
Expand All @@ -43,9 +43,9 @@
{actions.map((action, i) => {
if ('actions' in action) {
return (
<CMDKGroup key={i} display={action.display} keywords={action.keywords}>
<CMDKAction key={i} display={action.display} keywords={action.keywords}>
<ActionsToJSX actions={action.actions} />
</CMDKGroup>
</CMDKAction>
);
}
if ('to' in action) {
Expand Down Expand Up @@ -86,7 +86,7 @@
const handleAction = useCallback(
(action: CollectionTreeNode<CMDKActionData>) => {
if ('to' in action) {
navigate(String(action.to));

Check failure on line 89 in static/app/components/commandPalette/ui/commandPalette.spec.tsx

View workflow job for this annotation

GitHub Actions / pre-commit lint

'action.to' may use Object's default stringification format ('[object Object]') when stringified

Check failure on line 89 in static/app/components/commandPalette/ui/commandPalette.spec.tsx

View workflow job for this annotation

GitHub Actions / eslint

'action.to' may use Object's default stringification format ('[object Object]') when stringified
} else if ('onAction' in action) {
action.onAction();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ import {ISSUE_TAXONOMY_CONFIG} from 'sentry/views/issueList/taxonomies';
import {useStarredIssueViews} from 'sentry/views/navigation/secondary/sections/issues/issueViews/useStarredIssueViews';
import {getUserOrgNavigationConfiguration} from 'sentry/views/settings/organization/userOrgNavigationConfiguration';

import {CMDKAction, CMDKGroup} from './cmdk';
import {CMDKAction} from './cmdk';
import {CommandPaletteSlot} from './commandPaletteSlot';

const DSN_ICONS: React.ReactElement[] = [
Expand Down Expand Up @@ -82,8 +82,8 @@ export function GlobalCommandPaletteActions() {

return (
<CommandPaletteSlot name="global">
<CMDKGroup display={{label: t('Go to...')}}>
<CMDKGroup display={{label: t('Issues'), icon: <IconIssues />}}>
<CMDKAction display={{label: t('Go to...')}}>
<CMDKAction display={{label: t('Issues'), icon: <IconIssues />}}>
<CMDKAction display={{label: t('Feed')}} to={`${prefix}/issues/`} />
{Object.values(ISSUE_TAXONOMY_CONFIG).map(config => (
<CMDKAction
Expand All @@ -104,9 +104,9 @@ export function GlobalCommandPaletteActions() {
to={`${prefix}/issues/views/${starredView.id}/`}
/>
))}
</CMDKGroup>
</CMDKAction>

<CMDKGroup display={{label: t('Explore'), icon: <IconCompass />}}>
<CMDKAction display={{label: t('Explore'), icon: <IconCompass />}}>
<CMDKAction display={{label: t('Traces')}} to={`${prefix}/explore/traces/`} />
{organization.features.includes('ourlogs-enabled') && (
<CMDKAction display={{label: t('Logs')}} to={`${prefix}/explore/logs/`} />
Expand Down Expand Up @@ -135,26 +135,26 @@ export function GlobalCommandPaletteActions() {
display={{label: t('All Queries')}}
to={`${prefix}/explore/saved-queries/`}
/>
</CMDKGroup>
</CMDKAction>

<CMDKGroup display={{label: t('Dashboards'), icon: <IconDashboard />}}>
<CMDKAction display={{label: t('Dashboards'), icon: <IconDashboard />}}>
<CMDKAction
display={{label: t('All Dashboards')}}
to={`${prefix}/dashboards/`}
/>
<CMDKGroup display={{label: t('Starred Dashboards'), icon: <IconStar />}}>
<CMDKAction display={{label: t('Starred Dashboards'), icon: <IconStar />}}>
{starredDashboards.map(dashboard => (
<CMDKAction
key={dashboard.id}
display={{label: dashboard.title, icon: <IconStar />}}
to={`${prefix}/dashboard/${dashboard.id}/`}
/>
))}
</CMDKGroup>
</CMDKGroup>
</CMDKAction>
</CMDKAction>

{organization.features.includes('performance-view') && (
<CMDKGroup display={{label: t('Insights'), icon: <IconGraph type="area" />}}>
<CMDKAction display={{label: t('Insights'), icon: <IconGraph type="area" />}}>
<CMDKAction
display={{label: t('Frontend')}}
to={`${prefix}/insights/${FRONTEND_LANDING_SUB_PATH}/`}
Expand Down Expand Up @@ -186,18 +186,18 @@ export function GlobalCommandPaletteActions() {
display={{label: t('All Projects')}}
to={`${prefix}/insights/projects/`}
/>
</CMDKGroup>
</CMDKAction>
)}

<CMDKGroup display={{label: t('Settings'), icon: <IconSettings />}}>
<CMDKAction display={{label: t('Settings'), icon: <IconSettings />}}>
{getUserOrgNavigationConfiguration().flatMap(section =>
section.items.map(item => (
<CMDKAction key={item.path} display={{label: item.title}} to={item.path} />
))
)}
</CMDKGroup>
</CMDKAction>

<CMDKGroup display={{label: t('Project Settings'), icon: <IconSettings />}}>
<CMDKAction display={{label: t('Project Settings'), icon: <IconSettings />}}>
{projects.map(project => (
<CMDKAction
key={project.id}
Expand All @@ -208,10 +208,10 @@ export function GlobalCommandPaletteActions() {
to={`/settings/${organization.slug}/projects/${project.slug}/`}
/>
))}
</CMDKGroup>
</CMDKGroup>
</CMDKAction>
</CMDKAction>

<CMDKGroup display={{label: t('Add')}}>
<CMDKAction display={{label: t('Add')}}>
<CMDKAction
display={{label: t('Create Dashboard'), icon: <IconAdd />}}
keywords={[t('add dashboard')]}
Expand All @@ -232,10 +232,10 @@ export function GlobalCommandPaletteActions() {
keywords={[t('team invite')]}
onAction={openInviteMembersModal}
/>
</CMDKGroup>
</CMDKAction>

<CMDKGroup display={{label: t('DSN')}} keywords={[t('client keys')]}>
<CMDKGroup
<CMDKAction display={{label: t('DSN')}} keywords={[t('client keys')]}>
<CMDKAction
display={{label: t('Project DSN Keys'), icon: <IconLock locked />}}
keywords={[t('client keys'), t('dsn keys')]}
>
Expand All @@ -250,9 +250,9 @@ export function GlobalCommandPaletteActions() {
to={`/settings/${organization.slug}/projects/${project.slug}/keys/`}
/>
))}
</CMDKGroup>
</CMDKAction>
{hasDsnLookup && (
<CMDKGroup
<CMDKAction
display={{
label: t('Reverse DSN lookup'),
details: t(
Expand Down Expand Up @@ -287,11 +287,11 @@ export function GlobalCommandPaletteActions() {
{(data: CommandPaletteAsyncResult[]) =>
data.map((item, i) => renderAsyncResult(item, i))
}
</CMDKGroup>
</CMDKAction>
)}
</CMDKGroup>
</CMDKAction>

<CMDKGroup display={{label: t('Help')}}>
<CMDKAction display={{label: t('Help')}}>
<CMDKAction
display={{label: t('Open Documentation'), icon: <IconDocs />}}
onAction={() => window.open('https://docs.sentry.io', '_blank', 'noreferrer')}
Expand All @@ -314,7 +314,7 @@ export function GlobalCommandPaletteActions() {
window.open('https://sentry.io/changelog/', '_blank', 'noreferrer')
}
/>
<CMDKGroup
<CMDKAction
display={{label: t('Search Results')}}
resource={(query: string): CMDKQueryOptions => {
return queryOptions({
Expand Down Expand Up @@ -353,11 +353,11 @@ export function GlobalCommandPaletteActions() {
{(data: CommandPaletteAsyncResult[]) =>
data.map((item, i) => renderAsyncResult(item, i))
}
</CMDKGroup>
</CMDKGroup>
</CMDKAction>
</CMDKAction>

<CMDKGroup display={{label: t('Interface')}}>
<CMDKGroup display={{label: t('Change Color Theme'), icon: <IconSettings />}}>
<CMDKAction display={{label: t('Interface')}}>
<CMDKAction display={{label: t('Change Color Theme'), icon: <IconSettings />}}>
<CMDKAction
display={{label: t('System')}}
onAction={async () => {
Expand All @@ -382,8 +382,8 @@ export function GlobalCommandPaletteActions() {
addSuccessMessage(t('Theme preference saved: Dark'));
}}
/>
</CMDKGroup>
</CMDKGroup>
</CMDKAction>
</CMDKAction>
</CommandPaletteSlot>
);
}
Loading
Loading