diff --git a/static/app/components/commandPalette/__stories__/components.tsx b/static/app/components/commandPalette/__stories__/components.tsx index e7d81df6b38a6f..3b22337bd6caf8 100644 --- a/static/app/components/commandPalette/__stories__/components.tsx +++ b/static/app/components/commandPalette/__stories__/components.tsx @@ -36,18 +36,17 @@ export function CommandPaletteDemo() { onAction={() => addSuccessMessage('Child action executed')} /> - - - addSuccessMessage('Select all')} - /> - addSuccessMessage('Deselect all')} - /> - - + + addSuccessMessage('Select all')} + /> + addSuccessMessage('Deselect all')} + /> + + ); } diff --git a/static/app/components/commandPalette/ui/cmdk.tsx b/static/app/components/commandPalette/ui/cmdk.tsx index 17f9f9b488d6a6..872ca708943599 100644 --- a/static/app/components/commandPalette/ui/cmdk.tsx +++ b/static/app/components/commandPalette/ui/cmdk.tsx @@ -49,6 +49,13 @@ 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 8162c82371c524..ff915596c85e17 100644 --- a/static/app/components/commandPalette/ui/commandPalette.spec.tsx +++ b/static/app/components/commandPalette/ui/commandPalette.spec.tsx @@ -461,15 +461,33 @@ 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( - - - - - - + + + + + ); @@ -483,14 +501,13 @@ describe('CommandPalette', () => { const onAction = jest.fn(); render( - - - - - ('onAction' in node ? node.onAction() : null)} - /> - + + + + + ('onAction' in node ? node.onAction() : null)} + /> ); @@ -501,12 +518,11 @@ describe('CommandPalette', () => { it('page slot action is displayed in the palette', async () => { render( - - - - - - + + + + + ); @@ -520,14 +536,13 @@ describe('CommandPalette', () => { const onAction = jest.fn(); render( - - - - - ('onAction' in node ? node.onAction() : null)} - /> - + + + + + ('onAction' in node ? node.onAction() : null)} + /> ); @@ -537,23 +552,22 @@ describe('CommandPalette', () => { it('page slot actions are rendered before global actions', async () => { // This test mirrors the real app structure: - // - Global actions are registered directly in CMDKCollection (e.g. from the nav sidebar) + // - Global actions are registered via from the nav // - Page-specific actions are registered via // // Expected: page slot actions appear first in the list, global actions second. - // The "page" outlet is rendered above the "global" outlet inside CommandPalette, - // so page slot actions should always take priority in the list order. + // The outlets are rendered in task→page→global DOM order (matching navigation), + // so compareDocumentPosition sorts them correctly. render( - - {/* Global action registered directly — simulates e.g. GlobalCommandPaletteActions */} + + - {/* Page-specific action portaled via the page slot */} - - - - - + + + + + ); @@ -566,18 +580,17 @@ describe('CommandPalette', () => { it('task < page < global ordering when all three slots are populated', async () => { render( - - - - - - - - - - - - + + + + + + + + + + + ); @@ -588,12 +601,10 @@ describe('CommandPalette', () => { expect(options[2]).toHaveAccessibleName('Global Action'); }); - it('actions passed as children to CommandPalette via global slot are not duplicated', async () => { - // This mirrors the real app setup in modal.tsx where GlobalCommandPaletteActions - // is passed as children to CommandPalette. Those actions use - // internally, which creates a circular portal: - // the consumer is rendered inside the global outlet div and then portals back to it. - // Registration must be idempotent so the slot→portal transition never yields duplicates. + 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. function ActionsViaGlobalSlot() { return ( @@ -605,9 +616,9 @@ describe('CommandPalette', () => { render( - - - + + + ); diff --git a/static/app/components/commandPalette/ui/commandPalette.tsx b/static/app/components/commandPalette/ui/commandPalette.tsx index 6ce5e67cf3eae3..35c9ac58ef4516 100644 --- a/static/app/components/commandPalette/ui/commandPalette.tsx +++ b/static/app/components/commandPalette/ui/commandPalette.tsx @@ -22,7 +22,6 @@ import {Text} from '@sentry/scraps/text'; import type {CMDKActionData} from 'sentry/components/commandPalette/ui/cmdk'; import {CMDKCollection} from 'sentry/components/commandPalette/ui/cmdk'; import type {CollectionTreeNode} from 'sentry/components/commandPalette/ui/collection'; -import {CommandPaletteSlot} from 'sentry/components/commandPalette/ui/commandPaletteSlot'; import { useCommandPaletteDispatch, useCommandPaletteState, @@ -66,7 +65,6 @@ type CMDKFlatItem = CollectionTreeNode & { interface CommandPaletteProps { onAction: (action: CollectionTreeNode) => void; - children?: React.ReactNode; } export function CommandPalette(props: CommandPaletteProps) { @@ -350,19 +348,6 @@ export function CommandPalette(props: CommandPaletteProps) { - - {p =>
} - - - {p =>
} - - - {p => ( -
- {props.children} -
- )} -
{treeState.collection.size === 0 ? ( ) : ( @@ -393,8 +378,8 @@ export function CommandPalette(props: CommandPaletteProps) { /** * Pre-sorts the root-level nodes by DOM position of their slot outlet element. - * Outlets are declared in priority order inside CommandPalette (task → page → global), - * so compareDocumentPosition gives the correct ordering for free. + * Outlets are rendered in task → page → global order inside CommandPaletteProvider, + * so compareDocumentPosition gives the correct priority 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/modal.spec.tsx b/static/app/components/commandPalette/ui/modal.spec.tsx index 65b8832aa59fc7..bd774168079b3d 100644 --- a/static/app/components/commandPalette/ui/modal.spec.tsx +++ b/static/app/components/commandPalette/ui/modal.spec.tsx @@ -18,11 +18,6 @@ jest.mock('@tanstack/react-virtual', () => ({ }, })); -// Avoid pulling in the full global actions tree (needs org context, feature flags, etc.) -jest.mock('sentry/components/commandPalette/ui/commandPaletteGlobalActions', () => ({ - GlobalCommandPaletteActions: () => null, -})); - import {render, screen, userEvent} from 'sentry-test/reactTestingLibrary'; import { @@ -52,6 +47,25 @@ 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. +function SlotOutlets() { + return ( +
+ + {p =>
} + + + {p =>
} + + + {p =>
} + +
+ ); +} + describe('CommandPaletteModal', () => { beforeEach(() => { jest.resetAllMocks(); @@ -67,6 +81,7 @@ describe('CommandPaletteModal', () => { render( + @@ -91,6 +106,7 @@ describe('CommandPaletteModal', () => { render( + diff --git a/static/app/components/commandPalette/ui/modal.tsx b/static/app/components/commandPalette/ui/modal.tsx index 11303e4982a394..79d49e2aa9539f 100644 --- a/static/app/components/commandPalette/ui/modal.tsx +++ b/static/app/components/commandPalette/ui/modal.tsx @@ -5,7 +5,6 @@ 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 {GlobalCommandPaletteActions} from 'sentry/components/commandPalette/ui/commandPaletteGlobalActions'; import type {Theme} from 'sentry/utils/theme'; import {normalizeUrl} from 'sentry/utils/url/normalizeUrl'; import {useNavigate} from 'sentry/utils/useNavigate'; @@ -32,9 +31,7 @@ export default function CommandPaletteModal({Body, closeModal}: ModalRenderProps return ( - - - + ); } diff --git a/static/app/views/navigation/index.tsx b/static/app/views/navigation/index.tsx index 5433ac62400abe..645d6599bee073 100644 --- a/static/app/views/navigation/index.tsx +++ b/static/app/views/navigation/index.tsx @@ -5,6 +5,8 @@ import {useHotkeys} from '@sentry/scraps/hotkey'; import {Container, Flex} from '@sentry/scraps/layout'; import {ExternalLink} from '@sentry/scraps/link'; +import {GlobalCommandPaletteActions} from 'sentry/components/commandPalette/ui/commandPaletteGlobalActions'; +import {CommandPaletteSlot} from 'sentry/components/commandPalette/ui/commandPaletteSlot'; import {CommandPaletteHotkeys} from 'sentry/components/commandPalette/ui/commandPaletteStateContext'; import {useGlobalModal} from 'sentry/components/globalModal/useGlobalModal'; import {t} from 'sentry/locale'; @@ -28,6 +30,29 @@ import { import {useHasPageFrameFeature} from 'sentry/views/navigation/useHasPageFrameFeature'; import {useResetActiveNavigationGroup} from 'sentry/views/navigation/useResetActiveNavigationGroup'; +/** + * Renders the CMDK slot outlet elements in task → page → global DOM order so + * that presortBySlotRef's compareDocumentPosition sorting works correctly. + * Keeping these in the navigation (rather than in CommandPaletteProvider) + * means they only exist when the full nav is mounted — tests that assert an + * empty container are unaffected. + */ +function CommandPaletteSlotOutlets() { + return ( +
+ + {p =>
} + + + {p =>
} + + + {p =>
} + +
+ ); +} + function UserAndOrganizationNavigation() { const {layout} = usePrimaryNavigation(); const {visible} = useGlobalModal(); @@ -49,6 +74,8 @@ function UserAndOrganizationNavigation() { return ( + + {layout === 'mobile' ? ( {hasPageFrame ? : }