diff --git a/static/app/components/commandPalette/__stories__/components.tsx b/static/app/components/commandPalette/__stories__/components.tsx index fb28a8e6d25dd8..069d99c71c5120 100644 --- a/static/app/components/commandPalette/__stories__/components.tsx +++ b/static/app/components/commandPalette/__stories__/components.tsx @@ -1,31 +1,11 @@ -import {useCallback} from 'react'; - import {addSuccessMessage} from 'sentry/actionCreators/indicator'; -import {CommandPaletteProvider} 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 { + CMDKAction, + CommandPaletteProvider, +} from 'sentry/components/commandPalette/ui/cmdk'; import {CommandPalette} from 'sentry/components/commandPalette/ui/commandPalette'; -import {normalizeUrl} from 'sentry/utils/url/normalizeUrl'; -import {useNavigate} from 'sentry/utils/useNavigate'; export function CommandPaletteDemo() { - const navigate = useNavigate(); - - const handleAction = useCallback( - ( - action: CollectionTreeNode, - _options?: {modifierKeys?: {shiftKey: boolean}} - ) => { - if ('to' in action) { - navigate(normalizeUrl(action.to)); - } else if ('onAction' in action) { - action.onAction(); - } - }, - [navigate] - ); - return ( @@ -49,7 +29,7 @@ export function CommandPaletteDemo() { onAction={() => addSuccessMessage('Deselect all')} /> - + ); } diff --git a/static/app/components/commandPalette/ui/cmdk.tsx b/static/app/components/commandPalette/ui/cmdk.tsx index 2735b7ff22cfff..9531e632e336a6 100644 --- a/static/app/components/commandPalette/ui/cmdk.tsx +++ b/static/app/components/commandPalette/ui/cmdk.tsx @@ -50,13 +50,6 @@ export const CMDKCollection = makeCollection(); /** * Root provider for the command palette. Wrap the component tree that * contains CMDKAction registrations and the CommandPalette UI. - * - * Slot outlets are rendered separately in the navigation (see - * CommandPaletteSlotOutlets in navigation/index.tsx) in task → page → global - * DOM order so that presortBySlotRef's compareDocumentPosition sorting works - * correctly. Keeping the outlets in the navigation (rather than here) means - * this provider introduces no DOM nodes — tests that assert an empty container - * are unaffected. */ export function CommandPaletteProvider({children}: {children: React.ReactNode}) { return ( diff --git a/static/app/components/commandPalette/ui/commandPalette.spec.tsx b/static/app/components/commandPalette/ui/commandPalette.spec.tsx index 87159ad41f92d3..b7192c10e0e429 100644 --- a/static/app/components/commandPalette/ui/commandPalette.spec.tsx +++ b/static/app/components/commandPalette/ui/commandPalette.spec.tsx @@ -1,4 +1,4 @@ -import {Fragment, useCallback} from 'react'; +import {Fragment} from 'react'; import {render, screen, userEvent, waitFor} from 'sentry-test/reactTestingLibrary'; @@ -26,51 +26,44 @@ import {closeModal} from 'sentry/actionCreators/modal'; import * as modalActions from 'sentry/actionCreators/modal'; import type {CommandPaletteAction} from 'sentry/components/commandPalette/types'; import {cmdkQueryOptions} from 'sentry/components/commandPalette/types'; -import {CommandPaletteProvider} 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 { + CMDKAction, + CommandPaletteProvider, +} from 'sentry/components/commandPalette/ui/cmdk'; import {CommandPalette} from 'sentry/components/commandPalette/ui/commandPalette'; import {CommandPaletteSlot} from 'sentry/components/commandPalette/ui/commandPaletteSlot'; -import {useNavigate} from 'sentry/utils/useNavigate'; - -function GlobalActionsComponent({ - children, - onAction, -}: { - children?: React.ReactNode; - onAction?: ( - action: CollectionTreeNode, - options?: {modifierKeys?: {shiftKey: boolean}} - ) => void; -}) { - const navigate = useNavigate(); - - const handleAction = useCallback( - ( - action: CollectionTreeNode, - options?: {modifierKeys?: {shiftKey: boolean}} - ) => { - if (onAction) { - onAction(action, options); - } else if ('to' in action) { - navigate(action.to); - } else if ('onAction' in action) { - action.onAction(); - } - closeModal(); - }, - [navigate, onAction] - ); +function GlobalActionsComponent({children}: {children?: React.ReactNode}) { return ( {children} - + ); } +/** + * Renders the slot outlets that live outside CommandPalette in the real app + * (they are mounted in navigation/index.tsx). Tests that use + * must include this component so slot consumers + * have a registered outlet element to portal into. + */ +function SlotOutlets() { + return ( +
+ + {p =>
} + + + {p =>
} + + + {p =>
} + +
+ ); +} + const onChild = jest.fn(); function AllActions() { @@ -120,12 +113,12 @@ describe('CommandPalette', () => { expect(closeSpy).toHaveBeenCalledTimes(1); }); - it('shift-enter on an internal link forwards modifier keys and closes modal', async () => { + it('shift-enter on an internal link opens in a new tab and closes modal', async () => { const closeSpy = jest.spyOn(modalActions, 'closeModal'); - const onAction = jest.fn(); + const openSpy = jest.spyOn(window, 'open').mockReturnValue(null); render( - + ); @@ -133,10 +126,14 @@ describe('CommandPalette', () => { await screen.findByRole('textbox', {name: 'Search commands'}); await userEvent.keyboard('{Shift>}{Enter}{/Shift}'); - expect(onAction).toHaveBeenCalledWith(expect.objectContaining({to: '/target/'}), { - modifierKeys: {shiftKey: true}, - }); + expect(openSpy).toHaveBeenCalledWith( + expect.stringContaining('target'), + '_blank', + 'noreferrer' + ); expect(closeSpy).toHaveBeenCalledTimes(1); + + openSpy.mockRestore(); }); it('shows internal and external trailing link indicators for link actions', async () => { @@ -400,18 +397,6 @@ describe('CommandPalette', () => { const secondaryCallback = jest.fn(); const closeSpy = jest.spyOn(modalActions, 'closeModal'); - // Mirror the updated modal.tsx handleSelect: invoke callback, skip close when - // action has children so the palette can push into the secondary actions. - const handleAction = (action: CollectionTreeNode) => { - if ('onAction' in action) { - action.onAction(); - if (action.children.length > 0) { - return; - } - } - closeModal(); - }; - // Top-level groups become section headers (disabled), so the action-with-callback // must be a child item — matching how "Parent Group Action" works in allActions. render( @@ -424,7 +409,7 @@ describe('CommandPalette', () => { /> - + ); @@ -475,7 +460,7 @@ describe('CommandPalette', () => { render( - + ); expect( @@ -638,33 +623,14 @@ describe('CommandPalette', () => { }); describe('slot rendering', () => { - // Outlets live in the navigation in production; tests that exercise slot - // behaviour must render them explicitly so slot consumers have a target to - // portal into. - function SlotOutlets() { - return ( -
- - {p =>
} - - - {p =>
} - - - {p =>
} - -
- ); - } - it('task slot action is displayed in the palette', async () => { render( - - + + ); @@ -678,13 +644,11 @@ describe('CommandPalette', () => { const onAction = jest.fn(); render( - - ('onAction' in node ? node.onAction() : null)} - /> + + ); @@ -695,11 +659,11 @@ describe('CommandPalette', () => { it('page slot action is displayed in the palette', async () => { render( - - + + ); @@ -713,13 +677,11 @@ describe('CommandPalette', () => { const onAction = jest.fn(); render( - - ('onAction' in node ? node.onAction() : null)} - /> + + ); @@ -729,22 +691,22 @@ describe('CommandPalette', () => { it('page slot actions are rendered before global actions', async () => { // This test mirrors the real app structure: - // - Global actions are registered via from the nav + // - Global actions are registered directly in CMDKCollection (e.g. from the nav sidebar) // - Page-specific actions are registered via // // Expected: page slot actions appear first in the list, global actions second. - // The outlets are rendered in task→page→global DOM order (matching navigation), - // so compareDocumentPosition sorts them correctly. + // The "page" outlet is rendered above the "global" outlet inside CommandPalette, + // so page slot actions should always take priority in the list order. render( - - - - + {/* Global action registered directly — simulates e.g. GlobalCommandPaletteActions */} + + {/* Page-specific action portaled via the page slot */} - + + ); @@ -757,7 +719,6 @@ describe('CommandPalette', () => { it('task < page < global ordering when all three slots are populated', async () => { render( - @@ -767,7 +728,8 @@ describe('CommandPalette', () => { - + + ); @@ -778,10 +740,10 @@ describe('CommandPalette', () => { expect(options[2]).toHaveAccessibleName('Global Action'); }); - it('global slot actions registered outside CommandPalette are not duplicated', async () => { - // Mirrors the real app setup where GlobalCommandPaletteActions lives in the nav - // (a sibling of CommandPalette, not a child), portaling into the global outlet. - // The collection registration must be idempotent. + it('actions registered via a slot consumer are not duplicated', async () => { + // GlobalCommandPaletteActions uses internally. + // The slot consumer portals children into the outlet element. Registration must be + // idempotent so the slot→portal transition never yields duplicates. function ActionsViaGlobalSlot() { return ( @@ -793,9 +755,9 @@ describe('CommandPalette', () => { render( - - + + ); @@ -812,7 +774,7 @@ describe('CommandPalette', () => { - + ); @@ -826,7 +788,7 @@ describe('CommandPalette', () => { render( - + ); @@ -855,7 +817,7 @@ describe('CommandPalette', () => { } - + ); @@ -878,7 +840,7 @@ describe('CommandPalette', () => { - + ); @@ -904,7 +866,7 @@ describe('CommandPalette', () => { - + ); @@ -928,7 +890,7 @@ describe('CommandPalette', () => { - + ); @@ -955,7 +917,7 @@ describe('CommandPalette', () => { } - + ); diff --git a/static/app/components/commandPalette/ui/commandPalette.tsx b/static/app/components/commandPalette/ui/commandPalette.tsx index 5ef4276ea8cc14..7d300f13177526 100644 --- a/static/app/components/commandPalette/ui/commandPalette.tsx +++ b/static/app/components/commandPalette/ui/commandPalette.tsx @@ -26,7 +26,10 @@ import { useCommandPaletteDispatch, useCommandPaletteState, } from 'sentry/components/commandPalette/ui/commandPaletteStateContext'; -import {isExternalLocation} from 'sentry/components/commandPalette/ui/locationUtils'; +import { + getLocationHref, + isExternalLocation, +} from 'sentry/components/commandPalette/ui/locationUtils'; import {useCommandPaletteAnalytics} from 'sentry/components/commandPalette/useCommandPaletteAnalytics'; import {FeedbackButton} from 'sentry/components/feedbackButton/feedbackButton'; import {LoadingIndicator} from 'sentry/components/loadingIndicator'; @@ -36,7 +39,9 @@ import {t} from 'sentry/locale'; import {useIsFetching} from 'sentry/utils/queryClient'; import {fzf} from 'sentry/utils/search/fzf'; import type {Theme} from 'sentry/utils/theme'; +import {normalizeUrl} from 'sentry/utils/url/normalizeUrl'; import {useDebouncedValue} from 'sentry/utils/useDebouncedValue'; +import {useNavigate} from 'sentry/utils/useNavigate'; const MotionButton = motion.create(Button); const MotionIconSearch = motion.create(IconSearch); @@ -66,14 +71,12 @@ type CMDKFlatItem = CollectionTreeNode & { }; interface CommandPaletteProps { - onAction: ( - action: CollectionTreeNode, - options?: {modifierKeys?: {shiftKey: boolean}} - ) => void; + closeModal?: () => void; } export function CommandPalette(props: CommandPaletteProps) { const theme = useTheme(); + const navigate = useNavigate(); const store = CMDKCollection.useStore(); const state = useCommandPaletteState(); @@ -204,6 +207,7 @@ export function CommandPalette(props: CommandPaletteProps) { disallowTypeAhead: true, }); + const {closeModal} = props; const onActionSelection = useCallback( ( key: string | number | null, @@ -241,9 +245,21 @@ export function CommandPalette(props: CommandPaletteProps) { analytics.recordAction(action, resultIndex, ''); dispatch({type: 'trigger action'}); - props.onAction(action, options); + + if ('to' in action) { + const normalizedTo = normalizeUrl(action.to); + if (isExternalLocation(normalizedTo) || options?.modifierKeys?.shiftKey) { + window.open(getLocationHref(normalizedTo), '_blank', 'noreferrer'); + } else { + navigate(normalizedTo); + } + } else if ('onAction' in action) { + action.onAction(); + } + + closeModal?.(); }, - [actions, analytics, dispatch, props] + [actions, analytics, closeModal, dispatch, navigate] ); const resultsListRef = useRef(null); @@ -432,8 +448,8 @@ export function CommandPalette(props: CommandPaletteProps) { /** * Pre-sorts the root-level nodes by DOM position of their slot outlet element. - * Outlets are rendered in task → page → global order inside CommandPaletteProvider, - * so compareDocumentPosition gives the correct priority ordering for free. + * Outlets are declared in priority order inside CommandPalette (task → page → global), + * so compareDocumentPosition gives the correct ordering for free. * Nodes sharing the same outlet (same slot) retain their existing relative order. * Nodes without a slot ref are not reordered relative to each other. */ diff --git a/static/app/components/commandPalette/ui/commandPaletteGlobalActions.tsx b/static/app/components/commandPalette/ui/commandPaletteGlobalActions.tsx index df07e645403817..ed066dc9dfbc8b 100644 --- a/static/app/components/commandPalette/ui/commandPaletteGlobalActions.tsx +++ b/static/app/components/commandPalette/ui/commandPaletteGlobalActions.tsx @@ -335,9 +335,7 @@ export function GlobalCommandPaletteActions() { }); }} > - {data => { - return data.map((item, i) => renderAsyncResult(item, i)); - }} + {data => data.map((item, i) => renderAsyncResult(item, i))} )} diff --git a/static/app/components/commandPalette/ui/modal.spec.tsx b/static/app/components/commandPalette/ui/modal.spec.tsx index d1c816eb119885..eba0780ff9a6f9 100644 --- a/static/app/components/commandPalette/ui/modal.spec.tsx +++ b/static/app/components/commandPalette/ui/modal.spec.tsx @@ -48,9 +48,12 @@ function makeRenderProps(closeModal: jest.Mock) { }; } -// Outlets live in the navigation in production; tests that exercise slot -// behaviour must render them explicitly so slot consumers have a target to -// portal into. +/** + * Renders the slot outlets that live outside CommandPalette in the real app + * (they are mounted in navigation/index.tsx). Tests that use + * must include this component so slot consumers + * have a registered outlet element to portal into. + */ function SlotOutlets() { return (
@@ -82,10 +85,10 @@ describe('CommandPaletteModal', () => { render( - + ); @@ -140,7 +143,6 @@ describe('CommandPaletteModal', () => { render( - @@ -148,6 +150,7 @@ describe('CommandPaletteModal', () => { + ); @@ -167,10 +170,10 @@ describe('CommandPaletteModal', () => { render( - + ); @@ -192,10 +195,10 @@ describe('CommandPaletteModal', () => { render( - + ); diff --git a/static/app/components/commandPalette/ui/modal.tsx b/static/app/components/commandPalette/ui/modal.tsx index edf80cbef6c07b..81479e8a8e9794 100644 --- a/static/app/components/commandPalette/ui/modal.tsx +++ b/static/app/components/commandPalette/ui/modal.tsx @@ -1,50 +1,13 @@ -import {useCallback} from 'react'; import {css} from '@emotion/react'; import type {ModalRenderProps} from 'sentry/actionCreators/modal'; -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'; -import { - getLocationHref, - isExternalLocation, -} from 'sentry/components/commandPalette/ui/locationUtils'; import type {Theme} from 'sentry/utils/theme'; -import {normalizeUrl} from 'sentry/utils/url/normalizeUrl'; -import {useNavigate} from 'sentry/utils/useNavigate'; export default function CommandPaletteModal({Body, closeModal}: ModalRenderProps) { - const navigate = useNavigate(); - - const handleSelect = useCallback( - ( - action: CollectionTreeNode, - options?: {modifierKeys?: {shiftKey: boolean}} - ) => { - if ('to' in action) { - const normalizedTo = normalizeUrl(action.to); - if (isExternalLocation(normalizedTo) || options?.modifierKeys?.shiftKey) { - window.open(getLocationHref(normalizedTo), '_blank', 'noreferrer'); - } else { - navigate(normalizedTo); - } - } else if ('onAction' in action) { - // When the action has children, the palette will push into them so the - // user can select a secondary action — keep the modal open. The - // callback already ran when the palette delegated this selection. - if (action.children.length > 0) { - return; - } - action.onAction(); - } - closeModal(); - }, - [navigate, closeModal] - ); - return ( - + ); } diff --git a/static/app/components/commandPalette/useCommandPaletteActions.mdx b/static/app/components/commandPalette/useCommandPaletteActions.mdx index 181048a4e64a1b..1b2737abc16f23 100644 --- a/static/app/components/commandPalette/useCommandPaletteActions.mdx +++ b/static/app/components/commandPalette/useCommandPaletteActions.mdx @@ -34,33 +34,14 @@ Wrap your tree in `CommandPaletteProvider` and place the `CommandPalette` UI com ```tsx -import {useCallback} from 'react'; - import {addSuccessMessage} from 'sentry/actionCreators/indicator'; import { CMDKAction, CommandPaletteProvider, } 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'; -import {normalizeUrl} from 'sentry/utils/url/normalizeUrl'; -import {useNavigate} from 'sentry/utils/useNavigate'; function YourComponent() { - const navigate = useNavigate(); - - const handleAction = useCallback( - (action: CollectionTreeNode) => { - if ('to' in action) { - navigate(normalizeUrl(String(action.to))); - } else if ('onAction' in action) { - action.onAction(); - } - }, - [navigate] - ); - return ( {/* Navigation action */} @@ -80,7 +61,7 @@ function YourComponent() { /> - {/* Inline actions declared as siblings to the palette */} + {/* Inline actions — register them directly inside the provider */} {/* The command palette UI */} - + ); }