Skip to content

Commit 88b2723

Browse files
JonasBaclaude
JonasBa
and
claude
authored
fix(cmdk): Keep CMDKAction nodes mounted across modal open/close cycles (#112650)
Move CommandPalette Slot outside of the conditionally rendered CommandPalette which makes the action registration stable and enables the user to toggle the modal on/off without losing state --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent cd2aa33 commit 88b2723

File tree

7 files changed

+140
-98
lines changed

7 files changed

+140
-98
lines changed

‎static/app/components/commandPalette/__stories__/components.tsx‎

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -36,18 +36,17 @@ export function CommandPaletteDemo() {
3636
onAction={() => addSuccessMessage('Child action executed')}
3737
/>
3838
</CMDKAction>
39-
<CommandPalette onAction={handleAction}>
40-
<CMDKAction display={{label: 'Issues List'}}>
41-
<CMDKAction
42-
display={{label: 'Select all'}}
43-
onAction={() => addSuccessMessage('Select all')}
44-
/>
45-
<CMDKAction
46-
display={{label: 'Deselect all'}}
47-
onAction={() => addSuccessMessage('Deselect all')}
48-
/>
49-
</CMDKAction>
50-
</CommandPalette>
39+
<CMDKAction display={{label: 'Issues List'}}>
40+
<CMDKAction
41+
display={{label: 'Select all'}}
42+
onAction={() => addSuccessMessage('Select all')}
43+
/>
44+
<CMDKAction
45+
display={{label: 'Deselect all'}}
46+
onAction={() => addSuccessMessage('Deselect all')}
47+
/>
48+
</CMDKAction>
49+
<CommandPalette onAction={handleAction} />
5150
</CommandPaletteProvider>
5251
);
5352
}

‎static/app/components/commandPalette/ui/cmdk.tsx‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,13 @@ export const CMDKCollection = makeCollection<CMDKActionData>();
4949
/**
5050
* Root provider for the command palette. Wrap the component tree that
5151
* contains CMDKAction registrations and the CommandPalette UI.
52+
*
53+
* Slot outlets are rendered separately in the navigation (see
54+
* CommandPaletteSlotOutlets in navigation/index.tsx) in task → page → global
55+
* DOM order so that presortBySlotRef's compareDocumentPosition sorting works
56+
* correctly. Keeping the outlets in the navigation (rather than here) means
57+
* this provider introduces no DOM nodes — tests that assert an empty container
58+
* are unaffected.
5259
*/
5360
export function CommandPaletteProvider({children}: {children: React.ReactNode}) {
5461
return (

‎static/app/components/commandPalette/ui/commandPalette.spec.tsx‎

Lines changed: 71 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -461,15 +461,33 @@ describe('CommandPalette', () => {
461461
});
462462

463463
describe('slot rendering', () => {
464+
// Outlets live in the navigation in production; tests that exercise slot
465+
// behaviour must render them explicitly so slot consumers have a target to
466+
// portal into.
467+
function SlotOutlets() {
468+
return (
469+
<div style={{display: 'none'}}>
470+
<CommandPaletteSlot.Outlet name="task">
471+
{p => <div {...p} />}
472+
</CommandPaletteSlot.Outlet>
473+
<CommandPaletteSlot.Outlet name="page">
474+
{p => <div {...p} />}
475+
</CommandPaletteSlot.Outlet>
476+
<CommandPaletteSlot.Outlet name="global">
477+
{p => <div {...p} />}
478+
</CommandPaletteSlot.Outlet>
479+
</div>
480+
);
481+
}
482+
464483
it('task slot action is displayed in the palette', async () => {
465484
render(
466485
<CommandPaletteProvider>
467-
<CommandPaletteSlot.Provider>
468-
<CommandPaletteSlot name="task">
469-
<CMDKAction display={{label: 'Task Action'}} onAction={jest.fn()} />
470-
</CommandPaletteSlot>
471-
<CommandPalette onAction={jest.fn()} />
472-
</CommandPaletteSlot.Provider>
486+
<SlotOutlets />
487+
<CommandPaletteSlot name="task">
488+
<CMDKAction display={{label: 'Task Action'}} onAction={jest.fn()} />
489+
</CommandPaletteSlot>
490+
<CommandPalette onAction={jest.fn()} />
473491
</CommandPaletteProvider>
474492
);
475493

@@ -483,14 +501,13 @@ describe('CommandPalette', () => {
483501
const onAction = jest.fn();
484502
render(
485503
<CommandPaletteProvider>
486-
<CommandPaletteSlot.Provider>
487-
<CommandPaletteSlot name="task">
488-
<CMDKAction display={{label: 'Task Action'}} onAction={onAction} />
489-
</CommandPaletteSlot>
490-
<CommandPalette
491-
onAction={node => ('onAction' in node ? node.onAction() : null)}
492-
/>
493-
</CommandPaletteSlot.Provider>
504+
<SlotOutlets />
505+
<CommandPaletteSlot name="task">
506+
<CMDKAction display={{label: 'Task Action'}} onAction={onAction} />
507+
</CommandPaletteSlot>
508+
<CommandPalette
509+
onAction={node => ('onAction' in node ? node.onAction() : null)}
510+
/>
494511
</CommandPaletteProvider>
495512
);
496513

@@ -501,12 +518,11 @@ describe('CommandPalette', () => {
501518
it('page slot action is displayed in the palette', async () => {
502519
render(
503520
<CommandPaletteProvider>
504-
<CommandPaletteSlot.Provider>
505-
<CommandPaletteSlot name="page">
506-
<CMDKAction display={{label: 'Page Action'}} onAction={jest.fn()} />
507-
</CommandPaletteSlot>
508-
<CommandPalette onAction={jest.fn()} />
509-
</CommandPaletteSlot.Provider>
521+
<SlotOutlets />
522+
<CommandPaletteSlot name="page">
523+
<CMDKAction display={{label: 'Page Action'}} onAction={jest.fn()} />
524+
</CommandPaletteSlot>
525+
<CommandPalette onAction={jest.fn()} />
510526
</CommandPaletteProvider>
511527
);
512528

@@ -520,14 +536,13 @@ describe('CommandPalette', () => {
520536
const onAction = jest.fn();
521537
render(
522538
<CommandPaletteProvider>
523-
<CommandPaletteSlot.Provider>
524-
<CommandPaletteSlot name="page">
525-
<CMDKAction display={{label: 'Page Action'}} onAction={onAction} />
526-
</CommandPaletteSlot>
527-
<CommandPalette
528-
onAction={node => ('onAction' in node ? node.onAction() : null)}
529-
/>
530-
</CommandPaletteSlot.Provider>
539+
<SlotOutlets />
540+
<CommandPaletteSlot name="page">
541+
<CMDKAction display={{label: 'Page Action'}} onAction={onAction} />
542+
</CommandPaletteSlot>
543+
<CommandPalette
544+
onAction={node => ('onAction' in node ? node.onAction() : null)}
545+
/>
531546
</CommandPaletteProvider>
532547
);
533548

@@ -537,23 +552,22 @@ describe('CommandPalette', () => {
537552

538553
it('page slot actions are rendered before global actions', async () => {
539554
// This test mirrors the real app structure:
540-
// - Global actions are registered directly in CMDKCollection (e.g. from the nav sidebar)
555+
// - Global actions are registered via <CommandPaletteSlot name="global"> from the nav
541556
// - Page-specific actions are registered via <CommandPaletteSlot name="page">
542557
//
543558
// Expected: page slot actions appear first in the list, global actions second.
544-
// The "page" outlet is rendered above the "global" outlet inside CommandPalette,
545-
// so page slot actions should always take priority in the list order.
559+
// The outlets are rendered in task→page→global DOM order (matching navigation),
560+
// so compareDocumentPosition sorts them correctly.
546561
render(
547562
<CommandPaletteProvider>
548-
<CommandPaletteSlot.Provider>
549-
{/* Global action registered directly — simulates e.g. GlobalCommandPaletteActions */}
563+
<SlotOutlets />
564+
<CommandPaletteSlot name="global">
550565
<CMDKAction display={{label: 'Global Action'}} onAction={jest.fn()} />
551-
{/* Page-specific action portaled via the page slot */}
552-
<CommandPaletteSlot name="page">
553-
<CMDKAction display={{label: 'Page Action'}} onAction={jest.fn()} />
554-
</CommandPaletteSlot>
555-
<CommandPalette onAction={jest.fn()} />
556-
</CommandPaletteSlot.Provider>
566+
</CommandPaletteSlot>
567+
<CommandPaletteSlot name="page">
568+
<CMDKAction display={{label: 'Page Action'}} onAction={jest.fn()} />
569+
</CommandPaletteSlot>
570+
<CommandPalette onAction={jest.fn()} />
557571
</CommandPaletteProvider>
558572
);
559573

@@ -566,18 +580,17 @@ describe('CommandPalette', () => {
566580
it('task < page < global ordering when all three slots are populated', async () => {
567581
render(
568582
<CommandPaletteProvider>
569-
<CommandPaletteSlot.Provider>
570-
<CommandPaletteSlot name="global">
571-
<CMDKAction display={{label: 'Global Action'}} onAction={jest.fn()} />
572-
</CommandPaletteSlot>
573-
<CommandPaletteSlot name="page">
574-
<CMDKAction display={{label: 'Page Action'}} onAction={jest.fn()} />
575-
</CommandPaletteSlot>
576-
<CommandPaletteSlot name="task">
577-
<CMDKAction display={{label: 'Task Action'}} onAction={jest.fn()} />
578-
</CommandPaletteSlot>
579-
<CommandPalette onAction={jest.fn()} />
580-
</CommandPaletteSlot.Provider>
583+
<SlotOutlets />
584+
<CommandPaletteSlot name="global">
585+
<CMDKAction display={{label: 'Global Action'}} onAction={jest.fn()} />
586+
</CommandPaletteSlot>
587+
<CommandPaletteSlot name="page">
588+
<CMDKAction display={{label: 'Page Action'}} onAction={jest.fn()} />
589+
</CommandPaletteSlot>
590+
<CommandPaletteSlot name="task">
591+
<CMDKAction display={{label: 'Task Action'}} onAction={jest.fn()} />
592+
</CommandPaletteSlot>
593+
<CommandPalette onAction={jest.fn()} />
581594
</CommandPaletteProvider>
582595
);
583596

@@ -588,12 +601,10 @@ describe('CommandPalette', () => {
588601
expect(options[2]).toHaveAccessibleName('Global Action');
589602
});
590603

591-
it('actions passed as children to CommandPalette via global slot are not duplicated', async () => {
592-
// This mirrors the real app setup in modal.tsx where GlobalCommandPaletteActions
593-
// is passed as children to CommandPalette. Those actions use
594-
// <CommandPaletteSlot name="global"> internally, which creates a circular portal:
595-
// the consumer is rendered inside the global outlet div and then portals back to it.
596-
// Registration must be idempotent so the slot→portal transition never yields duplicates.
604+
it('global slot actions registered outside CommandPalette are not duplicated', async () => {
605+
// Mirrors the real app setup where GlobalCommandPaletteActions lives in the nav
606+
// (a sibling of CommandPalette, not a child), portaling into the global outlet.
607+
// The collection registration must be idempotent.
597608
function ActionsViaGlobalSlot() {
598609
return (
599610
<CommandPaletteSlot name="global">
@@ -605,9 +616,9 @@ describe('CommandPalette', () => {
605616

606617
render(
607618
<CommandPaletteProvider>
608-
<CommandPalette onAction={jest.fn()}>
609-
<ActionsViaGlobalSlot />
610-
</CommandPalette>
619+
<SlotOutlets />
620+
<ActionsViaGlobalSlot />
621+
<CommandPalette onAction={jest.fn()} />
611622
</CommandPaletteProvider>
612623
);
613624

‎static/app/components/commandPalette/ui/commandPalette.tsx‎

Lines changed: 2 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ import {Text} from '@sentry/scraps/text';
2222
import type {CMDKActionData} from 'sentry/components/commandPalette/ui/cmdk';
2323
import {CMDKCollection} from 'sentry/components/commandPalette/ui/cmdk';
2424
import type {CollectionTreeNode} from 'sentry/components/commandPalette/ui/collection';
25-
import {CommandPaletteSlot} from 'sentry/components/commandPalette/ui/commandPaletteSlot';
2625
import {
2726
useCommandPaletteDispatch,
2827
useCommandPaletteState,
@@ -66,7 +65,6 @@ type CMDKFlatItem = CollectionTreeNode<CMDKActionData> & {
6665

6766
interface CommandPaletteProps {
6867
onAction: (action: CollectionTreeNode<CMDKActionData>) => void;
69-
children?: React.ReactNode;
7068
}
7169

7270
export function CommandPalette(props: CommandPaletteProps) {
@@ -350,19 +348,6 @@ export function CommandPalette(props: CommandPaletteProps) {
350348
</Flex>
351349
</Flex>
352350

353-
<CommandPaletteSlot.Outlet name="task">
354-
{p => <div {...p} style={{display: 'contents'}} />}
355-
</CommandPaletteSlot.Outlet>
356-
<CommandPaletteSlot.Outlet name="page">
357-
{p => <div {...p} style={{display: 'contents'}} />}
358-
</CommandPaletteSlot.Outlet>
359-
<CommandPaletteSlot.Outlet name="global">
360-
{p => (
361-
<div {...p} style={{display: 'contents'}}>
362-
{props.children}
363-
</div>
364-
)}
365-
</CommandPaletteSlot.Outlet>
366351
{treeState.collection.size === 0 ? (
367352
<CommandPaletteNoResults />
368353
) : (
@@ -393,8 +378,8 @@ export function CommandPalette(props: CommandPaletteProps) {
393378

394379
/**
395380
* Pre-sorts the root-level nodes by DOM position of their slot outlet element.
396-
* Outlets are declared in priority order inside CommandPalette (task → page → global),
397-
* so compareDocumentPosition gives the correct ordering for free.
381+
* Outlets are rendered in task → page → global order inside CommandPaletteProvider,
382+
* so compareDocumentPosition gives the correct priority ordering for free.
398383
* Nodes sharing the same outlet (same slot) retain their existing relative order.
399384
* Nodes without a slot ref are not reordered relative to each other.
400385
*/

‎static/app/components/commandPalette/ui/modal.spec.tsx‎

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,6 @@ jest.mock('@tanstack/react-virtual', () => ({
1818
},
1919
}));
2020

21-
// Avoid pulling in the full global actions tree (needs org context, feature flags, etc.)
22-
jest.mock('sentry/components/commandPalette/ui/commandPaletteGlobalActions', () => ({
23-
GlobalCommandPaletteActions: () => null,
24-
}));
25-
2621
import {render, screen, userEvent} from 'sentry-test/reactTestingLibrary';
2722

2823
import {
@@ -52,6 +47,25 @@ function makeRenderProps(closeModal: jest.Mock) {
5247
};
5348
}
5449

50+
// Outlets live in the navigation in production; tests that exercise slot
51+
// behaviour must render them explicitly so slot consumers have a target to
52+
// portal into.
53+
function SlotOutlets() {
54+
return (
55+
<div style={{display: 'none'}}>
56+
<CommandPaletteSlot.Outlet name="task">
57+
{p => <div {...p} />}
58+
</CommandPaletteSlot.Outlet>
59+
<CommandPaletteSlot.Outlet name="page">
60+
{p => <div {...p} />}
61+
</CommandPaletteSlot.Outlet>
62+
<CommandPaletteSlot.Outlet name="global">
63+
{p => <div {...p} />}
64+
</CommandPaletteSlot.Outlet>
65+
</div>
66+
);
67+
}
68+
5569
describe('CommandPaletteModal', () => {
5670
beforeEach(() => {
5771
jest.resetAllMocks();
@@ -67,6 +81,7 @@ describe('CommandPaletteModal', () => {
6781

6882
render(
6983
<CommandPaletteProvider>
84+
<SlotOutlets />
7085
<CommandPaletteSlot name="task">
7186
<CMDKAction display={{label: 'Leaf Action'}} onAction={onActionSpy} />
7287
</CommandPaletteSlot>
@@ -91,6 +106,7 @@ describe('CommandPaletteModal', () => {
91106

92107
render(
93108
<CommandPaletteProvider>
109+
<SlotOutlets />
94110
<CommandPaletteSlot name="task">
95111
<CMDKAction display={{label: 'Outer Group'}}>
96112
<CMDKAction display={{label: 'Parent Action'}} onAction={onActionSpy}>

‎static/app/components/commandPalette/ui/modal.tsx‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import type {ModalRenderProps} from 'sentry/actionCreators/modal';
55
import type {CMDKActionData} from 'sentry/components/commandPalette/ui/cmdk';
66
import type {CollectionTreeNode} from 'sentry/components/commandPalette/ui/collection';
77
import {CommandPalette} from 'sentry/components/commandPalette/ui/commandPalette';
8-
import {GlobalCommandPaletteActions} from 'sentry/components/commandPalette/ui/commandPaletteGlobalActions';
98
import type {Theme} from 'sentry/utils/theme';
109
import {normalizeUrl} from 'sentry/utils/url/normalizeUrl';
1110
import {useNavigate} from 'sentry/utils/useNavigate';
@@ -32,9 +31,7 @@ export default function CommandPaletteModal({Body, closeModal}: ModalRenderProps
3231

3332
return (
3433
<Body>
35-
<CommandPalette onAction={handleSelect}>
36-
<GlobalCommandPaletteActions />
37-
</CommandPalette>
34+
<CommandPalette onAction={handleSelect} />
3835
</Body>
3936
);
4037
}

0 commit comments

Comments
 (0)