Skip to content

Commit 38f6481

Browse files
JonasBaclaude
JonasBa
and
claude
committed
ref(cmdk): Move slot outlets from CommandPaletteProvider into navigation
CommandPaletteProvider previously rendered a hidden div with three slot outlet elements. Because the global test wrapper includes CommandPaletteProvider, that hidden div showed up in every test's container, breaking tests that assert toBeEmptyDOMElement(). Move the outlets into UserAndOrganizationNavigation in navigation/index.tsx so they only exist when the full nav is mounted. CommandPaletteProvider becomes a pure context provider with no DOM output. Slot consumers and presortBySlotRef already degrade gracefully when outlets are absent, so tests unrelated to CMDK are unaffected. Slot-specific tests in commandPalette.spec.tsx and modal.spec.tsx now render a local SlotOutlets component explicitly, making their dependency on outlets visible rather than relying on a hidden side-effect in the provider. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 2dd2c88 commit 38f6481

File tree

4 files changed

+83
-27
lines changed

4 files changed

+83
-27
lines changed

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

Lines changed: 7 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -50,34 +50,18 @@ export const CMDKCollection = makeCollection<CMDKActionData>();
5050
* Root provider for the command palette. Wrap the component tree that
5151
* contains CMDKAction registrations and the CommandPalette UI.
5252
*
53-
* Slot outlets are rendered here in a hidden container so that slot consumers
54-
* (e.g. GlobalCommandPaletteActions in the nav, page-level action components)
55-
* always have an outlet to portal into — regardless of whether the modal is
56-
* open. This keeps CMDKAction nodes mounted and their useId() keys stable
57-
* across modal open/close cycles, preventing context loss when navigating into
58-
* a group and then closing and reopening the palette.
59-
*
60-
* The outlets are rendered in task → page → global DOM order so that
61-
* presortBySlotRef's compareDocumentPosition sorting still works correctly.
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.
6259
*/
6360
export function CommandPaletteProvider({children}: {children: React.ReactNode}) {
6461
return (
6562
<CommandPaletteStateProvider>
6663
<CommandPaletteSlot.Provider>
67-
<CMDKCollection.Provider>
68-
<div style={{display: 'none'}}>
69-
<CommandPaletteSlot.Outlet name="task">
70-
{p => <div {...p} />}
71-
</CommandPaletteSlot.Outlet>
72-
<CommandPaletteSlot.Outlet name="page">
73-
{p => <div {...p} />}
74-
</CommandPaletteSlot.Outlet>
75-
<CommandPaletteSlot.Outlet name="global">
76-
{p => <div {...p} />}
77-
</CommandPaletteSlot.Outlet>
78-
</div>
79-
{children}
80-
</CMDKCollection.Provider>
64+
<CMDKCollection.Provider>{children}</CMDKCollection.Provider>
8165
</CommandPaletteSlot.Provider>
8266
</CommandPaletteStateProvider>
8367
);

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

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -461,9 +461,29 @@ 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>
486+
<SlotOutlets />
467487
<CommandPaletteSlot name="task">
468488
<CMDKAction display={{label: 'Task Action'}} onAction={jest.fn()} />
469489
</CommandPaletteSlot>
@@ -481,6 +501,7 @@ describe('CommandPalette', () => {
481501
const onAction = jest.fn();
482502
render(
483503
<CommandPaletteProvider>
504+
<SlotOutlets />
484505
<CommandPaletteSlot name="task">
485506
<CMDKAction display={{label: 'Task Action'}} onAction={onAction} />
486507
</CommandPaletteSlot>
@@ -497,6 +518,7 @@ describe('CommandPalette', () => {
497518
it('page slot action is displayed in the palette', async () => {
498519
render(
499520
<CommandPaletteProvider>
521+
<SlotOutlets />
500522
<CommandPaletteSlot name="page">
501523
<CMDKAction display={{label: 'Page Action'}} onAction={jest.fn()} />
502524
</CommandPaletteSlot>
@@ -514,6 +536,7 @@ describe('CommandPalette', () => {
514536
const onAction = jest.fn();
515537
render(
516538
<CommandPaletteProvider>
539+
<SlotOutlets />
517540
<CommandPaletteSlot name="page">
518541
<CMDKAction display={{label: 'Page Action'}} onAction={onAction} />
519542
</CommandPaletteSlot>
@@ -533,10 +556,11 @@ describe('CommandPalette', () => {
533556
// - Page-specific actions are registered via <CommandPaletteSlot name="page">
534557
//
535558
// Expected: page slot actions appear first in the list, global actions second.
536-
// The hidden outlets in CommandPaletteProvider are rendered in task→page→global
537-
// DOM order, so compareDocumentPosition sorts them correctly.
559+
// The outlets are rendered in task→page→global DOM order (matching navigation),
560+
// so compareDocumentPosition sorts them correctly.
538561
render(
539562
<CommandPaletteProvider>
563+
<SlotOutlets />
540564
<CommandPaletteSlot name="global">
541565
<CMDKAction display={{label: 'Global Action'}} onAction={jest.fn()} />
542566
</CommandPaletteSlot>
@@ -556,6 +580,7 @@ describe('CommandPalette', () => {
556580
it('task < page < global ordering when all three slots are populated', async () => {
557581
render(
558582
<CommandPaletteProvider>
583+
<SlotOutlets />
559584
<CommandPaletteSlot name="global">
560585
<CMDKAction display={{label: 'Global Action'}} onAction={jest.fn()} />
561586
</CommandPaletteSlot>
@@ -578,8 +603,8 @@ describe('CommandPalette', () => {
578603

579604
it('global slot actions registered outside CommandPalette are not duplicated', async () => {
580605
// Mirrors the real app setup where GlobalCommandPaletteActions lives in the nav
581-
// (a sibling of CommandPalette, not a child), portaling into the hidden global
582-
// outlet in CommandPaletteProvider. The collection registration must be idempotent.
606+
// (a sibling of CommandPalette, not a child), portaling into the global outlet.
607+
// The collection registration must be idempotent.
583608
function ActionsViaGlobalSlot() {
584609
return (
585610
<CommandPaletteSlot name="global">
@@ -591,6 +616,7 @@ describe('CommandPalette', () => {
591616

592617
render(
593618
<CommandPaletteProvider>
619+
<SlotOutlets />
594620
<ActionsViaGlobalSlot />
595621
<CommandPalette onAction={jest.fn()} />
596622
</CommandPaletteProvider>

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,25 @@ function makeRenderProps(closeModal: jest.Mock) {
4747
};
4848
}
4949

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+
5069
describe('CommandPaletteModal', () => {
5170
beforeEach(() => {
5271
jest.resetAllMocks();
@@ -62,6 +81,7 @@ describe('CommandPaletteModal', () => {
6281

6382
render(
6483
<CommandPaletteProvider>
84+
<SlotOutlets />
6585
<CommandPaletteSlot name="task">
6686
<CMDKAction display={{label: 'Leaf Action'}} onAction={onActionSpy} />
6787
</CommandPaletteSlot>
@@ -86,6 +106,7 @@ describe('CommandPaletteModal', () => {
86106

87107
render(
88108
<CommandPaletteProvider>
109+
<SlotOutlets />
89110
<CommandPaletteSlot name="task">
90111
<CMDKAction display={{label: 'Outer Group'}}>
91112
<CMDKAction display={{label: 'Parent Action'}} onAction={onActionSpy}>

‎static/app/views/navigation/index.tsx‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {Container, Flex} from '@sentry/scraps/layout';
66
import {ExternalLink} from '@sentry/scraps/link';
77

88
import {GlobalCommandPaletteActions} from 'sentry/components/commandPalette/ui/commandPaletteGlobalActions';
9+
import {CommandPaletteSlot} from 'sentry/components/commandPalette/ui/commandPaletteSlot';
910
import {CommandPaletteHotkeys} from 'sentry/components/commandPalette/ui/commandPaletteStateContext';
1011
import {useGlobalModal} from 'sentry/components/globalModal/useGlobalModal';
1112
import {t} from 'sentry/locale';
@@ -29,6 +30,29 @@ import {
2930
import {useHasPageFrameFeature} from 'sentry/views/navigation/useHasPageFrameFeature';
3031
import {useResetActiveNavigationGroup} from 'sentry/views/navigation/useResetActiveNavigationGroup';
3132

33+
/**
34+
* Renders the CMDK slot outlet elements in task → page → global DOM order so
35+
* that presortBySlotRef's compareDocumentPosition sorting works correctly.
36+
* Keeping these in the navigation (rather than in CommandPaletteProvider)
37+
* means they only exist when the full nav is mounted — tests that assert an
38+
* empty container are unaffected.
39+
*/
40+
function CommandPaletteSlotOutlets() {
41+
return (
42+
<div style={{display: 'none'}}>
43+
<CommandPaletteSlot.Outlet name="task">
44+
{p => <div {...p} />}
45+
</CommandPaletteSlot.Outlet>
46+
<CommandPaletteSlot.Outlet name="page">
47+
{p => <div {...p} />}
48+
</CommandPaletteSlot.Outlet>
49+
<CommandPaletteSlot.Outlet name="global">
50+
{p => <div {...p} />}
51+
</CommandPaletteSlot.Outlet>
52+
</div>
53+
);
54+
}
55+
3256
function UserAndOrganizationNavigation() {
3357
const {layout} = usePrimaryNavigation();
3458
const {visible} = useGlobalModal();
@@ -50,6 +74,7 @@ function UserAndOrganizationNavigation() {
5074
return (
5175
<NavigationLayout>
5276
<CommandPaletteHotkeys />
77+
<CommandPaletteSlotOutlets />
5378
<GlobalCommandPaletteActions />
5479
{layout === 'mobile' ? (
5580
<MobileSecondaryNavigationContextProvider>

0 commit comments

Comments
 (0)