Skip to content

fix(cmdk): Keep CMDKAction nodes mounted across modal open/close cycles #112650

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JonasBa merged 2 commits into from
Apr 10, 2026
Merged

fix(cmdk): Keep CMDKAction nodes mounted across modal open/close cycles #112650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2dd2c88
fix(cmdk): Keep CMDKAction nodes mounted across modal open/close cycles
JonasBa Apr 10, 2026
38f6481
ref(cmdk): Move slot outlets from CommandPaletteProvider into navigation
JonasBa Apr 10, 2026
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
23 changes: 11 additions & 12 deletions static/app/components/commandPalette/__stories__/components.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,17 @@ export function CommandPaletteDemo() {
onAction={() => addSuccessMessage('Child action executed')}
/>
</CMDKAction>
<CommandPalette onAction={handleAction}>
<CMDKAction display={{label: 'Issues List'}}>
<CMDKAction
display={{label: 'Select all'}}
onAction={() => addSuccessMessage('Select all')}
/>
<CMDKAction
display={{label: 'Deselect all'}}
onAction={() => addSuccessMessage('Deselect all')}
/>
</CMDKAction>
</CommandPalette>
<CMDKAction display={{label: 'Issues List'}}>
<CMDKAction
display={{label: 'Select all'}}
onAction={() => addSuccessMessage('Select all')}
/>
<CMDKAction
display={{label: 'Deselect all'}}
onAction={() => addSuccessMessage('Deselect all')}
/>
</CMDKAction>
<CommandPalette onAction={handleAction} />
</CommandPaletteProvider>
);
}
7 changes: 7 additions & 0 deletions static/app/components/commandPalette/ui/cmdk.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,13 @@ export const CMDKCollection = makeCollection<CMDKActionData>();
/**
* 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 (
Expand Down
131 changes: 71 additions & 60 deletions static/app/components/commandPalette/ui/commandPalette.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
<div style={{display: 'none'}}>
<CommandPaletteSlot.Outlet name="task">
{p => <div {...p} />}
</CommandPaletteSlot.Outlet>
<CommandPaletteSlot.Outlet name="page">
{p => <div {...p} />}
</CommandPaletteSlot.Outlet>
<CommandPaletteSlot.Outlet name="global">
{p => <div {...p} />}
</CommandPaletteSlot.Outlet>
</div>
);
}

it('task slot action is displayed in the palette', async () => {
render(
<CommandPaletteProvider>
<CommandPaletteSlot.Provider>
<CommandPaletteSlot name="task">
<CMDKAction display={{label: 'Task Action'}} onAction={jest.fn()} />
</CommandPaletteSlot>
<CommandPalette onAction={jest.fn()} />
</CommandPaletteSlot.Provider>
<SlotOutlets />
<CommandPaletteSlot name="task">
<CMDKAction display={{label: 'Task Action'}} onAction={jest.fn()} />
</CommandPaletteSlot>
<CommandPalette onAction={jest.fn()} />
</CommandPaletteProvider>
);

Expand All @@ -483,14 +501,13 @@ describe('CommandPalette', () => {
const onAction = jest.fn();
render(
<CommandPaletteProvider>
<CommandPaletteSlot.Provider>
<CommandPaletteSlot name="task">
<CMDKAction display={{label: 'Task Action'}} onAction={onAction} />
</CommandPaletteSlot>
<CommandPalette
onAction={node => ('onAction' in node ? node.onAction() : null)}
/>
</CommandPaletteSlot.Provider>
<SlotOutlets />
<CommandPaletteSlot name="task">
<CMDKAction display={{label: 'Task Action'}} onAction={onAction} />
</CommandPaletteSlot>
<CommandPalette
onAction={node => ('onAction' in node ? node.onAction() : null)}
/>
</CommandPaletteProvider>
);

Expand All @@ -501,12 +518,11 @@ describe('CommandPalette', () => {
it('page slot action is displayed in the palette', async () => {
render(
<CommandPaletteProvider>
<CommandPaletteSlot.Provider>
<CommandPaletteSlot name="page">
<CMDKAction display={{label: 'Page Action'}} onAction={jest.fn()} />
</CommandPaletteSlot>
<CommandPalette onAction={jest.fn()} />
</CommandPaletteSlot.Provider>
<SlotOutlets />
<CommandPaletteSlot name="page">
<CMDKAction display={{label: 'Page Action'}} onAction={jest.fn()} />
</CommandPaletteSlot>
<CommandPalette onAction={jest.fn()} />
</CommandPaletteProvider>
);

Expand All @@ -520,14 +536,13 @@ describe('CommandPalette', () => {
const onAction = jest.fn();
render(
<CommandPaletteProvider>
<CommandPaletteSlot.Provider>
<CommandPaletteSlot name="page">
<CMDKAction display={{label: 'Page Action'}} onAction={onAction} />
</CommandPaletteSlot>
<CommandPalette
onAction={node => ('onAction' in node ? node.onAction() : null)}
/>
</CommandPaletteSlot.Provider>
<SlotOutlets />
<CommandPaletteSlot name="page">
<CMDKAction display={{label: 'Page Action'}} onAction={onAction} />
</CommandPaletteSlot>
<CommandPalette
onAction={node => ('onAction' in node ? node.onAction() : null)}
/>
</CommandPaletteProvider>
);

Expand All @@ -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 <CommandPaletteSlot name="global"> from the nav
// - Page-specific actions are registered via <CommandPaletteSlot name="page">
//
// 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(
<CommandPaletteProvider>
<CommandPaletteSlot.Provider>
{/* Global action registered directly — simulates e.g. GlobalCommandPaletteActions */}
<SlotOutlets />
<CommandPaletteSlot name="global">
<CMDKAction display={{label: 'Global Action'}} onAction={jest.fn()} />
{/* Page-specific action portaled via the page slot */}
<CommandPaletteSlot name="page">
<CMDKAction display={{label: 'Page Action'}} onAction={jest.fn()} />
</CommandPaletteSlot>
<CommandPalette onAction={jest.fn()} />
</CommandPaletteSlot.Provider>
</CommandPaletteSlot>
<CommandPaletteSlot name="page">
<CMDKAction display={{label: 'Page Action'}} onAction={jest.fn()} />
</CommandPaletteSlot>
<CommandPalette onAction={jest.fn()} />
</CommandPaletteProvider>
);

Expand All @@ -566,18 +580,17 @@ describe('CommandPalette', () => {
it('task < page < global ordering when all three slots are populated', async () => {
render(
<CommandPaletteProvider>
<CommandPaletteSlot.Provider>
<CommandPaletteSlot name="global">
<CMDKAction display={{label: 'Global Action'}} onAction={jest.fn()} />
</CommandPaletteSlot>
<CommandPaletteSlot name="page">
<CMDKAction display={{label: 'Page Action'}} onAction={jest.fn()} />
</CommandPaletteSlot>
<CommandPaletteSlot name="task">
<CMDKAction display={{label: 'Task Action'}} onAction={jest.fn()} />
</CommandPaletteSlot>
<CommandPalette onAction={jest.fn()} />
</CommandPaletteSlot.Provider>
<SlotOutlets />
<CommandPaletteSlot name="global">
<CMDKAction display={{label: 'Global Action'}} onAction={jest.fn()} />
</CommandPaletteSlot>
<CommandPaletteSlot name="page">
<CMDKAction display={{label: 'Page Action'}} onAction={jest.fn()} />
</CommandPaletteSlot>
<CommandPaletteSlot name="task">
<CMDKAction display={{label: 'Task Action'}} onAction={jest.fn()} />
</CommandPaletteSlot>
<CommandPalette onAction={jest.fn()} />
</CommandPaletteProvider>
);

Expand All @@ -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
// <CommandPaletteSlot name="global"> 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 (
<CommandPaletteSlot name="global">
Expand All @@ -605,9 +616,9 @@ describe('CommandPalette', () => {

render(
<CommandPaletteProvider>
<CommandPalette onAction={jest.fn()}>
<ActionsViaGlobalSlot />
</CommandPalette>
<SlotOutlets />
<ActionsViaGlobalSlot />
<CommandPalette onAction={jest.fn()} />
</CommandPaletteProvider>
);

Expand Down
19 changes: 2 additions & 17 deletions static/app/components/commandPalette/ui/commandPalette.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -66,7 +65,6 @@ type CMDKFlatItem = CollectionTreeNode<CMDKActionData> & {

interface CommandPaletteProps {
onAction: (action: CollectionTreeNode<CMDKActionData>) => void;
children?: React.ReactNode;
}

export function CommandPalette(props: CommandPaletteProps) {
Expand Down Expand Up @@ -350,19 +348,6 @@ export function CommandPalette(props: CommandPaletteProps) {
</Flex>
</Flex>

<CommandPaletteSlot.Outlet name="task">
{p => <div {...p} style={{display: 'contents'}} />}
</CommandPaletteSlot.Outlet>
<CommandPaletteSlot.Outlet name="page">
{p => <div {...p} style={{display: 'contents'}} />}
</CommandPaletteSlot.Outlet>
<CommandPaletteSlot.Outlet name="global">
{p => (
<div {...p} style={{display: 'contents'}}>
{props.children}
</div>
)}
</CommandPaletteSlot.Outlet>
{treeState.collection.size === 0 ? (
<CommandPaletteNoResults />
) : (
Expand Down Expand Up @@ -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.
*/
Expand Down
26 changes: 21 additions & 5 deletions static/app/components/commandPalette/ui/modal.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 (
<div style={{display: 'none'}}>
<CommandPaletteSlot.Outlet name="task">
{p => <div {...p} />}
</CommandPaletteSlot.Outlet>
<CommandPaletteSlot.Outlet name="page">
{p => <div {...p} />}
</CommandPaletteSlot.Outlet>
<CommandPaletteSlot.Outlet name="global">
{p => <div {...p} />}
</CommandPaletteSlot.Outlet>
</div>
);
}

describe('CommandPaletteModal', () => {
beforeEach(() => {
jest.resetAllMocks();
Expand All @@ -67,6 +81,7 @@ describe('CommandPaletteModal', () => {

render(
<CommandPaletteProvider>
<SlotOutlets />
<CommandPaletteSlot name="task">
<CMDKAction display={{label: 'Leaf Action'}} onAction={onActionSpy} />
</CommandPaletteSlot>
Expand All @@ -91,6 +106,7 @@ describe('CommandPaletteModal', () => {

render(
<CommandPaletteProvider>
<SlotOutlets />
<CommandPaletteSlot name="task">
<CMDKAction display={{label: 'Outer Group'}}>
<CMDKAction display={{label: 'Parent Action'}} onAction={onActionSpy}>
Expand Down
5 changes: 1 addition & 4 deletions static/app/components/commandPalette/ui/modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -32,9 +31,7 @@ export default function CommandPaletteModal({Body, closeModal}: ModalRenderProps

return (
<Body>
<CommandPalette onAction={handleSelect}>
<GlobalCommandPaletteActions />
</CommandPalette>
<CommandPalette onAction={handleSelect} />
</Body>
);
}
Expand Down
Loading
Loading