From 2dd2c88052729657674b836bff1e25952f06353b Mon Sep 17 00:00:00 2001 From: JonasBa Date: Fri, 10 Apr 2026 07:49:09 +0200 Subject: [PATCH 1/2] fix(cmdk): Keep CMDKAction nodes mounted across modal open/close cycles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Selecting a group action and then closing and reopening the command palette caused the palette to show an empty state. The nav stack stored a useId() key that was tied to the CMDKAction component instance. When the modal closed, those instances unmounted and unregistered their keys from the collection store. On reopen, new instances generated new keys, leaving the stored key pointing to nothing. Fix by moving the slot outlets from CommandPalette (mounted only when the modal is open) into CommandPaletteProvider (always mounted). The outlets are rendered in a display:none container in task → page → global DOM order so that presortBySlotRef's compareDocumentPosition ordering is preserved. GlobalCommandPaletteActions is moved to the navigation component where it has org context and is always mounted, portaling into the provider's persistent global outlet. As a side effect, page-slot actions registered from page components are also stable across modal cycles for the same reason. Co-Authored-By: Claude Sonnet 4.6 --- .../commandPalette/__stories__/components.tsx | 23 ++-- .../app/components/commandPalette/ui/cmdk.tsx | 25 ++++- .../commandPalette/ui/commandPalette.spec.tsx | 105 ++++++++---------- .../commandPalette/ui/commandPalette.tsx | 19 +--- .../commandPalette/ui/modal.spec.tsx | 5 - .../components/commandPalette/ui/modal.tsx | 5 +- static/app/views/navigation/index.tsx | 2 + 7 files changed, 85 insertions(+), 99 deletions(-) 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..5a5baaf20b13b5 100644 --- a/static/app/components/commandPalette/ui/cmdk.tsx +++ b/static/app/components/commandPalette/ui/cmdk.tsx @@ -49,12 +49,35 @@ 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 here in a hidden container so that slot consumers + * (e.g. GlobalCommandPaletteActions in the nav, page-level action components) + * always have an outlet to portal into — regardless of whether the modal is + * open. This keeps CMDKAction nodes mounted and their useId() keys stable + * across modal open/close cycles, preventing context loss when navigating into + * a group and then closing and reopening the palette. + * + * The outlets are rendered in task → page → global DOM order so that + * presortBySlotRef's compareDocumentPosition sorting still works correctly. */ export function CommandPaletteProvider({children}: {children: React.ReactNode}) { return ( - {children} + +
+ + {p =>
} + + + {p =>
} + + + {p =>
} + +
+ {children} + ); diff --git a/static/app/components/commandPalette/ui/commandPalette.spec.tsx b/static/app/components/commandPalette/ui/commandPalette.spec.tsx index 8162c82371c524..977a5c2cf6da46 100644 --- a/static/app/components/commandPalette/ui/commandPalette.spec.tsx +++ b/static/app/components/commandPalette/ui/commandPalette.spec.tsx @@ -464,12 +464,10 @@ describe('CommandPalette', () => { it('task slot action is displayed in the palette', async () => { render( - - - - - - + + + + ); @@ -483,14 +481,12 @@ describe('CommandPalette', () => { const onAction = jest.fn(); render( - - - - - ('onAction' in node ? node.onAction() : null)} - /> - + + + + ('onAction' in node ? node.onAction() : null)} + /> ); @@ -501,12 +497,10 @@ describe('CommandPalette', () => { it('page slot action is displayed in the palette', async () => { render( - - - - - - + + + + ); @@ -520,14 +514,12 @@ describe('CommandPalette', () => { const onAction = jest.fn(); render( - - - - - ('onAction' in node ? node.onAction() : null)} - /> - + + + + ('onAction' in node ? node.onAction() : null)} + /> ); @@ -537,23 +529,21 @@ 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 hidden outlets in CommandPaletteProvider are rendered in task→page→global + // DOM order, so compareDocumentPosition sorts them correctly. render( - - {/* Global action registered directly — simulates e.g. GlobalCommandPaletteActions */} + - {/* Page-specific action portaled via the page slot */} - - - - - + + + + + ); @@ -566,18 +556,16 @@ describe('CommandPalette', () => { it('task < page < global ordering when all three slots are populated', async () => { render( - - - - - - - - - - - - + + + + + + + + + + ); @@ -588,12 +576,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 hidden global + // outlet in CommandPaletteProvider. The collection registration must be idempotent. function ActionsViaGlobalSlot() { return ( @@ -605,9 +591,8 @@ 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..6776c45e97b2b1 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 { 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..ee29184908d413 100644 --- a/static/app/views/navigation/index.tsx +++ b/static/app/views/navigation/index.tsx @@ -5,6 +5,7 @@ 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 {CommandPaletteHotkeys} from 'sentry/components/commandPalette/ui/commandPaletteStateContext'; import {useGlobalModal} from 'sentry/components/globalModal/useGlobalModal'; import {t} from 'sentry/locale'; @@ -49,6 +50,7 @@ function UserAndOrganizationNavigation() { return ( + {layout === 'mobile' ? ( {hasPageFrame ? : } From 38f648149d9e120128d60f4f3a29fb94dc1dc6f2 Mon Sep 17 00:00:00 2001 From: JonasBa Date: Fri, 10 Apr 2026 10:16:23 +0200 Subject: [PATCH 2/2] 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 --- .../app/components/commandPalette/ui/cmdk.tsx | 30 ++++------------ .../commandPalette/ui/commandPalette.spec.tsx | 34 ++++++++++++++++--- .../commandPalette/ui/modal.spec.tsx | 21 ++++++++++++ static/app/views/navigation/index.tsx | 25 ++++++++++++++ 4 files changed, 83 insertions(+), 27 deletions(-) diff --git a/static/app/components/commandPalette/ui/cmdk.tsx b/static/app/components/commandPalette/ui/cmdk.tsx index 5a5baaf20b13b5..872ca708943599 100644 --- a/static/app/components/commandPalette/ui/cmdk.tsx +++ b/static/app/components/commandPalette/ui/cmdk.tsx @@ -50,34 +50,18 @@ 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 here in a hidden container so that slot consumers - * (e.g. GlobalCommandPaletteActions in the nav, page-level action components) - * always have an outlet to portal into — regardless of whether the modal is - * open. This keeps CMDKAction nodes mounted and their useId() keys stable - * across modal open/close cycles, preventing context loss when navigating into - * a group and then closing and reopening the palette. - * - * The outlets are rendered in task → page → global DOM order so that - * presortBySlotRef's compareDocumentPosition sorting still works correctly. + * 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 ( - -
- - {p =>
} - - - {p =>
} - - - {p =>
} - -
- {children} - + {children} ); diff --git a/static/app/components/commandPalette/ui/commandPalette.spec.tsx b/static/app/components/commandPalette/ui/commandPalette.spec.tsx index 977a5c2cf6da46..ff915596c85e17 100644 --- a/static/app/components/commandPalette/ui/commandPalette.spec.tsx +++ b/static/app/components/commandPalette/ui/commandPalette.spec.tsx @@ -461,9 +461,29 @@ 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( + @@ -481,6 +501,7 @@ describe('CommandPalette', () => { const onAction = jest.fn(); render( + @@ -497,6 +518,7 @@ describe('CommandPalette', () => { it('page slot action is displayed in the palette', async () => { render( + @@ -514,6 +536,7 @@ describe('CommandPalette', () => { const onAction = jest.fn(); render( + @@ -533,10 +556,11 @@ describe('CommandPalette', () => { // - Page-specific actions are registered via // // Expected: page slot actions appear first in the list, global actions second. - // The hidden outlets in CommandPaletteProvider are rendered in task→page→global - // DOM order, so compareDocumentPosition sorts them correctly. + // The outlets are rendered in task→page→global DOM order (matching navigation), + // so compareDocumentPosition sorts them correctly. render( + @@ -556,6 +580,7 @@ describe('CommandPalette', () => { it('task < page < global ordering when all three slots are populated', async () => { render( + @@ -578,8 +603,8 @@ describe('CommandPalette', () => { 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 hidden global - // outlet in CommandPaletteProvider. The collection registration must be idempotent. + // (a sibling of CommandPalette, not a child), portaling into the global outlet. + // The collection registration must be idempotent. function ActionsViaGlobalSlot() { return ( @@ -591,6 +616,7 @@ describe('CommandPalette', () => { render( + diff --git a/static/app/components/commandPalette/ui/modal.spec.tsx b/static/app/components/commandPalette/ui/modal.spec.tsx index 6776c45e97b2b1..bd774168079b3d 100644 --- a/static/app/components/commandPalette/ui/modal.spec.tsx +++ b/static/app/components/commandPalette/ui/modal.spec.tsx @@ -47,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(); @@ -62,6 +81,7 @@ describe('CommandPaletteModal', () => { render( + @@ -86,6 +106,7 @@ describe('CommandPaletteModal', () => { render( + diff --git a/static/app/views/navigation/index.tsx b/static/app/views/navigation/index.tsx index ee29184908d413..645d6599bee073 100644 --- a/static/app/views/navigation/index.tsx +++ b/static/app/views/navigation/index.tsx @@ -6,6 +6,7 @@ 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'; @@ -29,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(); @@ -50,6 +74,7 @@ function UserAndOrganizationNavigation() { return ( + {layout === 'mobile' ? (