feat(nav): Allow Cmd+K to toggle command palette open and closed#111441
feat(nav): Allow Cmd+K to toggle command palette open and closed#111441
Conversation
Previously Cmd+K could only open the command palette. Pressing it again while the palette was open had no effect because the hotkey was disabled when any modal was visible. Now Cmd+K toggles the new command palette (behind cmd-k-supercharged flag): - Tracks whether the command palette is the active modal via a ref - Closes the palette if it's already open, opens it if no modal is open - Does nothing if a different modal is open - Uses includeInputs so the shortcut works while the search input is focused - Persists the search query across open/close cycles via a module-level variable
github-actions
bot
added
the
Scope: Frontend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed:
includeInputsunintentionally changes deprecated palette shortcut behavior- I scoped
includeInputsto only the supercharged path so deprecated palette shortcuts are again ignored while typing in inputs.
- I scoped
- ✅ Fixed: Ref not reset when modal is replaced, not closed
- I track the active modal renderer and clear command-palette-open state when the renderer changes while visible, preventing Cmd+K from closing unrelated replacement modals.
Or push these changes by commenting:
@cursor push 0f27e0eac8
Preview (0f27e0eac8)
diff --git a/static/app/views/navigation/index.desktop.spec.tsx b/static/app/views/navigation/index.desktop.spec.tsx
--- a/static/app/views/navigation/index.desktop.spec.tsx
+++ b/static/app/views/navigation/index.desktop.spec.tsx
@@ -13,7 +13,9 @@
type RouterConfig,
} from 'sentry-test/reactTestingLibrary';
+import * as ModalActionCreators from 'sentry/actionCreators/modal';
import {ConfigStore} from 'sentry/stores/configStore';
+import {ModalStore} from 'sentry/stores/modalStore';
import {Navigation} from 'sentry/views/navigation';
import {NAVIGATION_SIDEBAR_COLLAPSED_LOCAL_STORAGE_KEY} from 'sentry/views/navigation/constants';
import {PrimaryNavigationContextProvider} from 'sentry/views/navigation/primaryNavigationContext';
@@ -860,4 +862,68 @@
});
});
});
+
+ describe('command palette hotkeys', () => {
+ it('does not open deprecated command palette when an input has focus', async () => {
+ const openCommandPaletteDeprecatedSpy = jest
+ .spyOn(ModalActionCreators, 'openCommandPaletteDeprecated')
+ .mockResolvedValue();
+
+ const input = document.createElement('input');
+ document.body.appendChild(input);
+ input.focus();
+
+ render(
+ <PrimaryNavigationContextProvider>
+ <Navigation />
+ </PrimaryNavigationContextProvider>,
+ navigationContext()
+ );
+
+ await userEvent.keyboard('{Control>}k{/Control}');
+
+ expect(openCommandPaletteDeprecatedSpy).not.toHaveBeenCalled();
+
+ openCommandPaletteDeprecatedSpy.mockRestore();
+ input.remove();
+ });
+
+ it('does not close a replacement modal after command palette is replaced', async () => {
+ ModalStore.closeModal();
+
+ const commandPaletteRenderer = jest.fn(() => null);
+ const replacementModalRenderer = jest.fn(() => null);
+ const openCommandPaletteSpy = jest
+ .spyOn(ModalActionCreators, 'openCommandPalette')
+ .mockImplementation(() => {
+ ModalStore.openModal(commandPaletteRenderer, {});
+ return Promise.resolve();
+ });
+
+ render(
+ <PrimaryNavigationContextProvider>
+ <Navigation />
+ </PrimaryNavigationContextProvider>,
+ navigationContext({
+ organization: {
+ features: [...ALL_AVAILABLE_FEATURES, 'cmd-k-supercharged'],
+ },
+ })
+ );
+
+ await userEvent.keyboard('{Control>}k{/Control}');
+ expect(ModalStore.getState().renderer).toBe(commandPaletteRenderer);
+
+ ModalStore.openModal(replacementModalRenderer, {});
+ await waitFor(() => {
+ expect(ModalStore.getState().renderer).toBe(replacementModalRenderer);
+ });
+
+ await userEvent.keyboard('{Control>}k{/Control}');
+ expect(ModalStore.getState().renderer).toBe(replacementModalRenderer);
+
+ openCommandPaletteSpy.mockRestore();
+ ModalStore.closeModal();
+ });
+ });
});
diff --git a/static/app/views/navigation/index.tsx b/static/app/views/navigation/index.tsx
--- a/static/app/views/navigation/index.tsx
+++ b/static/app/views/navigation/index.tsx
@@ -33,30 +33,50 @@
function UserAndOrganizationNavigation() {
const organization = useOrganization();
const {layout} = usePrimaryNavigation();
- const {visible} = useGlobalModal();
+ const {visible, renderer} = useGlobalModal();
const hasPageFrame = useHasPageFrameFeature();
useGlobalCommandPaletteActions();
const commandPaletteOpenRef = useRef(false);
+ const commandPaletteRendererRef = useRef<typeof renderer>(null);
+ const hasSuperchargedCommandPalette =
+ organization.features.includes('cmd-k-supercharged');
useEffect(() => {
if (!visible) {
commandPaletteOpenRef.current = false;
+ commandPaletteRendererRef.current = null;
+ return;
}
- }, [visible]);
+ if (!commandPaletteOpenRef.current || !renderer) {
+ return;
+ }
+
+ if (commandPaletteRendererRef.current === null) {
+ commandPaletteRendererRef.current = renderer;
+ return;
+ }
+
+ if (commandPaletteRendererRef.current !== renderer) {
+ commandPaletteOpenRef.current = false;
+ commandPaletteRendererRef.current = null;
+ }
+ }, [visible, renderer]);
+
useHotkeys([
{
match: ['command+shift+p', 'command+k', 'ctrl+shift+p', 'ctrl+k'],
- includeInputs: true,
+ includeInputs: hasSuperchargedCommandPalette,
callback: () => {
- if (organization.features.includes('cmd-k-supercharged')) {
+ if (hasSuperchargedCommandPalette) {
if (visible && commandPaletteOpenRef.current) {
closeModal();
} else if (!visible) {
openCommandPalette();
commandPaletteOpenRef.current = true;
+ commandPaletteRendererRef.current = null;
}
} else if (!visible) {
openCommandPaletteDeprecated();This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
| useHotkeys([ | ||
| { | ||
| match: ['command+shift+p', 'command+k', 'ctrl+shift+p', 'ctrl+k'], | ||
| includeInputs: true, |
There was a problem hiding this comment.
includeInputs unintentionally changes deprecated palette shortcut behavior
Medium Severity
The includeInputs: true option is set on the single hotkey definition that covers both the supercharged and deprecated palette paths. Previously, the shortcut was suppressed when an input or textarea had focus. Now, for users without the cmd-k-supercharged flag, pressing Cmd+K while typing in any text field will unexpectedly open the deprecated command palette. The PR states the deprecated palette retains its existing behavior, but this changes how the shortcut is triggered.
Additional Locations (1)
| if (!visible) { | ||
| commandPaletteOpenRef.current = false; | ||
| } | ||
| }, [visible]); |
There was a problem hiding this comment.
Ref not reset when modal is replaced, not closed
Medium Severity
commandPaletteOpenRef is only reset when visible becomes false, but ModalStore.openModal directly replaces the renderer without clearing it first, so visible stays true when one modal replaces another. If the command palette is open and an action opens a different modal, commandPaletteOpenRef.current remains true. A subsequent Cmd+K press would then incorrectly call closeModal() on the unrelated modal instead of doing nothing.
Additional Locations (1)
There was a problem hiding this comment.
might use a query selector to find out if its currently on the screen or not
jshchnz
force-pushed
the
feat/cmd-k-toggle
branch
from
2fce036 to
76cd835
Compare
| const [query, _setQuery] = useState(persistedQuery); | ||
| const setQuery = useCallback((newQuery: string) => { | ||
| persistedQuery = newQuery; | ||
| _setQuery(newQuery); | ||
| }, []); |
There was a problem hiding this comment.
might be able to use useSessionStorage here
There was a problem hiding this comment.
would likely want to clear the string when closing normally
|
|
||
| export function useCommandPaletteState() { | ||
| const [query, setQuery] = useState(''); | ||
| const [query, _setQuery] = useState(persistedQuery); |
There was a problem hiding this comment.
We should hoist cmd+k state into a context provider outside of the modal, then we get the query string storage for free, and all we need to observe is the toggle behavior.
@rbro112 might know better, but iirc we already have a global cmd k provider where we could hoist this state into
|
@jshchnz @ryan953 my implementation here achieves the state persistence by hoisting state. I think we should use that state to store the active focus ref, and return the focus as well since it will be durable and persist across renders (it also means we can isolate that piece of logic away from the UI itself) |
|
This issue has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you remove the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
3 participants
Summary
cmd-k-superchargedfeature flag; the deprecated palette retains its existing open-only behaviorHow it works
includeInputs: trueensures the shortcut fires even while the palette's search input is focuseduseCommandPaletteStatepersists the search query across mount/unmount cyclesTest plan
cmd-k-superchargedflag → old palette opens only, no toggle behavior