Skip to content

Commit e2b0710

Browse files
JonasBaclaude
andcommitted
fix(cmdk): Use render prop closeModal to properly reset open state
The modal was calling the imported closeModal() from actionCreators/modal directly, which goes straight to ModalStore.closeModal() and bypasses GlobalModal's internal closeModal wrapper. That wrapper is the only place options.onClose is invoked — which is the closeCommandPaletteModal callback that dispatches {type: 'toggle modal'} to reset state.open. With state.open stuck at true after an action closed the palette, the next hotkey press would see open===true and treat it as a close request rather than an open request, requiring a second press to actually reopen. Fix by destructuring closeModal from ModalRenderProps and using that instead of the imported version. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 497dd7e commit e2b0710

File tree

2 files changed

+115
-3
lines changed

2 files changed

+115
-3
lines changed
Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
jest.unmock('lodash/debounce');
2+
3+
jest.mock('@tanstack/react-virtual', () => ({
4+
useVirtualizer: ({count}: {count: number}) => {
5+
const virtualItems = Array.from({length: count}, (_, index) => ({
6+
key: index,
7+
index,
8+
start: index * 48,
9+
size: 48,
10+
lane: 0,
11+
}));
12+
return {
13+
getVirtualItems: () => virtualItems,
14+
getTotalSize: () => count * 48,
15+
measureElement: jest.fn(),
16+
measure: jest.fn(),
17+
};
18+
},
19+
}));
20+
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+
26+
import {render, screen, userEvent} from 'sentry-test/reactTestingLibrary';
27+
28+
import {
29+
CMDKAction,
30+
CommandPaletteProvider,
31+
} from 'sentry/components/commandPalette/ui/cmdk';
32+
import {CommandPaletteSlot} from 'sentry/components/commandPalette/ui/commandPaletteSlot';
33+
import CommandPaletteModal from 'sentry/components/commandPalette/ui/modal';
34+
import {
35+
makeCloseButton,
36+
makeClosableHeader,
37+
ModalBody,
38+
ModalFooter,
39+
} from 'sentry/components/globalModal/components';
40+
41+
/**
42+
* Returns a minimal but valid ModalRenderProps with a jest.fn() as closeModal.
43+
* Header and CloseButton are wired to the same spy so all close paths are tracked.
44+
*/
45+
function makeRenderProps(closeModal: jest.Mock) {
46+
return {
47+
closeModal,
48+
Body: ModalBody,
49+
Footer: ModalFooter,
50+
Header: makeClosableHeader(closeModal),
51+
CloseButton: makeCloseButton(closeModal),
52+
};
53+
}
54+
55+
describe('CommandPaletteModal', () => {
56+
beforeEach(() => {
57+
jest.resetAllMocks();
58+
});
59+
60+
it('calls the render prop closeModal (not the imported one) when a leaf action is selected', async () => {
61+
// This test guards against the regression where modal.tsx called the
62+
// imported closeModal() directly. That bypasses GlobalModal's internal
63+
// closeModal(), so options.onClose never fires and state.open stays true —
64+
// causing the hotkey to need two presses to reopen the palette.
65+
const closeModalSpy = jest.fn();
66+
const onActionSpy = jest.fn();
67+
68+
render(
69+
<CommandPaletteProvider>
70+
<CommandPaletteSlot name="task">
71+
<CMDKAction display={{label: 'Leaf Action'}} onAction={onActionSpy} />
72+
</CommandPaletteSlot>
73+
<CommandPaletteModal {...makeRenderProps(closeModalSpy)} />
74+
</CommandPaletteProvider>
75+
);
76+
77+
await userEvent.click(await screen.findByRole('option', {name: 'Leaf Action'}));
78+
79+
// The action callback must fire …
80+
expect(onActionSpy).toHaveBeenCalledTimes(1);
81+
// … and the render-prop closeModal (which triggers options.onClose and
82+
// resets state.open) must be the one that is called, not an internally
83+
// imported closeModal that skips the onClose hook.
84+
expect(closeModalSpy).toHaveBeenCalledTimes(1);
85+
});
86+
87+
it('does not call closeModal when an action with children is selected', async () => {
88+
// Actions with children push into secondary actions — the modal stays open.
89+
const closeModalSpy = jest.fn();
90+
const onActionSpy = jest.fn();
91+
92+
render(
93+
<CommandPaletteProvider>
94+
<CommandPaletteSlot name="task">
95+
<CMDKAction display={{label: 'Outer Group'}}>
96+
<CMDKAction display={{label: 'Parent Action'}} onAction={onActionSpy}>
97+
<CMDKAction display={{label: 'Child Action'}} onAction={jest.fn()} />
98+
</CMDKAction>
99+
</CMDKAction>
100+
</CommandPaletteSlot>
101+
<CommandPaletteModal {...makeRenderProps(closeModalSpy)} />
102+
</CommandPaletteProvider>
103+
);
104+
105+
await userEvent.click(await screen.findByRole('option', {name: 'Parent Action'}));
106+
107+
expect(onActionSpy).toHaveBeenCalledTimes(1);
108+
// Modal must remain open so the user can select a secondary action
109+
expect(closeModalSpy).not.toHaveBeenCalled();
110+
// Secondary action is now visible
111+
expect(await screen.findByRole('option', {name: 'Child Action'})).toBeInTheDocument();
112+
});
113+
});

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ import {useCallback} from 'react';
22
import {css} from '@emotion/react';
33

44
import type {ModalRenderProps} from 'sentry/actionCreators/modal';
5-
import {closeModal} from 'sentry/actionCreators/modal';
65
import type {CMDKActionData} from 'sentry/components/commandPalette/ui/cmdk';
76
import type {CollectionTreeNode} from 'sentry/components/commandPalette/ui/collection';
87
import {CommandPalette} from 'sentry/components/commandPalette/ui/commandPalette';
@@ -11,7 +10,7 @@ import type {Theme} from 'sentry/utils/theme';
1110
import {normalizeUrl} from 'sentry/utils/url/normalizeUrl';
1211
import {useNavigate} from 'sentry/utils/useNavigate';
1312

14-
export default function CommandPaletteModal({Body}: ModalRenderProps) {
13+
export default function CommandPaletteModal({Body, closeModal}: ModalRenderProps) {
1514
const navigate = useNavigate();
1615

1716
const handleSelect = useCallback(
@@ -28,7 +27,7 @@ export default function CommandPaletteModal({Body}: ModalRenderProps) {
2827
}
2928
closeModal();
3029
},
31-
[navigate]
30+
[navigate, closeModal]
3231
);
3332

3433
return (

0 commit comments

Comments
 (0)