Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions static/app/utils/useHotkeys.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,50 @@ describe('useHotkeys', () => {
expect(callback).toHaveBeenCalled();
});

it('registers on capture phase with useCapture', () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test mock overwrites bubble handler with capture handler

High Severity

The test mock for document.addEventListener stores callbacks by event name (events[event] = callback), so only one callback survives per event. Since useHotkeys now registers two 'keydown' listeners (bubble then capture), the capture handler overwrites the bubble handler in events. All existing tests that invoke events.keydown!(evt) with non-useCapture hotkeys now silently run the capture handler, which skips those hotkeys due to the !!hotkey.useCapture !== capture guard. This breaks every pre-existing test case that expects a bubble-phase callback to fire.

Additional Locations (1)
Fix in Cursor Fix in Web

const callback = jest.fn();

renderHook(p => useHotkeys(p), {
initialProps: [{match: 'command+k', callback, useCapture: true}],
});

expect(document.addEventListener).toHaveBeenCalledWith(
'keydown',
expect.any(Function),
true
);

const captureCall = (document.addEventListener as jest.Mock).mock.calls.find(
call => call[0] === 'keydown' && call[2] === true
);
expect(captureCall).toBeDefined();

const captureHandler = captureCall[1];
const evt = makeKeyEventFixture('k', {metaKey: true});
captureHandler(evt);

expect(callback).toHaveBeenCalled();
});

it('does not fire capture hotkeys on bubble phase', () => {
const callback = jest.fn();

renderHook(p => useHotkeys(p), {
initialProps: [{match: 'command+k', callback, useCapture: true}],
});

const bubbleCall = (document.addEventListener as jest.Mock).mock.calls.find(
call => call[0] === 'keydown' && call[2] !== true
);
expect(bubbleCall).toBeDefined();

const bubbleHandler = bubbleCall[1];
const evt = makeKeyEventFixture('k', {metaKey: true});
bubbleHandler(evt);

expect(callback).not.toHaveBeenCalled();
});

it('skips preventDefault', () => {
const callback = jest.fn();

Expand Down
17 changes: 16 additions & 1 deletion static/app/utils/useHotkeys.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,12 @@ type Hotkey = {
* Do not call preventDefault on the keydown event
*/
skipPreventDefault?: boolean;
/**
* Register the listener on the capture phase instead of the bubble phase.
* Use this when the shortcut must fire even if a child component calls
* `stopPropagation()` on the keydown event (e.g., the search query builder).
*/
useCapture?: boolean;
};

/**
Expand All @@ -63,8 +69,12 @@ export function useHotkeys(hotkeys: Hotkey[]): void {
});

useEffect(() => {
const onKeyDown = (evt: KeyboardEvent) => {
const makeHandler = (capture: boolean) => (evt: KeyboardEvent) => {
for (const hotkey of hotkeysRef.current) {
if (!!hotkey.useCapture !== capture) {
continue;
}

const preventDefault = !hotkey.skipPreventDefault;
const keysets = toArray(hotkey.match).map(keys => keys.toLowerCase());

Expand Down Expand Up @@ -92,10 +102,15 @@ export function useHotkeys(hotkeys: Hotkey[]): void {
}
};

const onKeyDown = makeHandler(false);
const onKeyDownCapture = makeHandler(true);

document.addEventListener('keydown', onKeyDown);
document.addEventListener('keydown', onKeyDownCapture, true);

return () => {
document.removeEventListener('keydown', onKeyDown);
document.removeEventListener('keydown', onKeyDownCapture, true);
};
}, []);
}
2 changes: 2 additions & 0 deletions static/app/views/navigation/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ function UserAndOrganizationNavigation() {
: [
{
match: ['command+shift+p', 'command+k', 'ctrl+shift+p', 'ctrl+k'],
includeInputs: true,
useCapture: true,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we start with just includeInputs for now?

callback: () => {
if (organization.features.includes('cmd-k-supercharged')) {
openCommandPalette();
Expand Down
Loading