Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
9e916c2
[DevTools] hotkey to start/stop profiling
emily8rown Nov 13, 2025
138fb2c
Fixed linting issue
emily8rown Nov 17, 2025
144b936
use useEffectEvent to avoid unneccessary resubscription
emily8rown Nov 17, 2025
344d8a5
changed strict equality check to abstract
emily8rown Nov 19, 2025
a793da8
add check navigator variable is defined before use
emily8rown Nov 19, 2025
fa8526a
fixed imports
emily8rown Nov 19, 2025
fde4bff
Check if navigator is defined in profiler
emily8rown Nov 19, 2025
0205bd0
Merge branch 'facebook:main' into devtools-profiling-hot-key
emily8rown Nov 20, 2025
d4f533a
Test for the hotkey to toggle profiling
emily8rown Nov 21, 2025
9e756af
Fix linting issues
emily8rown Nov 21, 2025
3f397de
Fix linting errors
emily8rown Nov 21, 2025
de7e0c2
Comment spelling correction
emily8rown Nov 21, 2025
5180a8f
removed polling for hotkey test
emily8rown Nov 21, 2025
b405364
Removed unecessary comment
emily8rown Nov 21, 2025
3424b9a
replaced timout and promise with utils.act
emily8rown Nov 21, 2025
6c0d2b7
Switch test to use fake timers and turn recursively flush off
emily8rown Nov 21, 2025
daf937c
Update comment for profiler hotkey test
emily8rown Nov 21, 2025
3a2759a
switched to async for dispatching key event in the test
emily8rown Nov 21, 2025
bde0b30
switch test to utils.act for rendering profiler
emily8rown Nov 21, 2025
f62931b
Corrected comment
emily8rown Nov 21, 2025
501fe42
Merge branch 'devtools-profiling-hot-key' of https://github.com/emily…
emily8rown Nov 21, 2025
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
Original file line number Diff line number Diff line change
Expand Up @@ -584,4 +584,101 @@ describe('ProfilerContext', () => {
await utils.actAsync(() => context.selectFiber(childID, 'Child'));
expect(inspectedElementID).toBe(parentID);
});

it('should toggle profiling when the keyboard shortcut is pressed', async () => {
// Use real timers for this test to avoid conflicts with Timeline's setInterval
jest.useRealTimers();

// Conetext providers
Comment thread
emily8rown marked this conversation as resolved.
Outdated
const Profiler =
require('react-devtools-shared/src/devtools/views/Profiler/Profiler').default;
const {
TimelineContextController,
} = require('react-devtools-timeline/src/TimelineContext');
const {
SettingsContextController,
} = require('react-devtools-shared/src/devtools/views/Settings/SettingsContext');
const {
ModalDialogContextController,
} = require('react-devtools-shared/src/devtools/views/ModalDialog');

// need a dom component for profiling to be enabled
const Component = () => null;
utils.act(() => render(<Component />));

const profilerContainer = document.createElement('div');
document.body.appendChild(profilerContainer);

// Create a root for the profiler
const profilerRoot = ReactDOMClient.createRoot(profilerContainer);

try {
// Render the profiler - use React.act to ensure render completes
React.act(() => {

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.

Can you use utils.actAsync here? This should also allow removing the Promise on line 632.

profilerRoot.render(
<Contexts>
<SettingsContextController browserTheme="light">
<ModalDialogContextController>
<TimelineContextController>
<Profiler />
</TimelineContextController>
</ModalDialogContextController>
</SettingsContextController>
</Contexts>,
);
});

// Wait for any async effects to complete
await new Promise(resolve => setTimeout(resolve, 100));

// Verify that the profiler is not profiling.
expect(store.profilerStore.isProfilingBasedOnUserInput).toBe(false);

// Trigger the keyboard shortcut.
const ownerWindow = profilerContainer.ownerDocument.defaultView;
const isMac =
typeof navigator !== 'undefined' &&
navigator.platform.toUpperCase().indexOf('MAC') >= 0;

const keyEvent = new KeyboardEvent('keydown', {
key: 'e',
metaKey: isMac,
ctrlKey: !isMac,
bubbles: true,
});

// Helper to wait for profiling state to change
const waitForProfilingState = async (expectedState, timeoutMs = 1000) => {
const startTime = Date.now();
while (
store.profilerStore.isProfilingBasedOnUserInput !== expectedState
) {
if (Date.now() - startTime > timeoutMs) {
throw new Error(
`Timeout waiting for profiling to be ${expectedState}. ` +
`Current state: ${store.profilerStore.isProfilingBasedOnUserInput}`,
);
}
await new Promise(resolve => setTimeout(resolve, 10));
}
};

// Dispatch keyboard event to toggle profiling on
ownerWindow.dispatchEvent(keyEvent);
await waitForProfilingState(true);

@hoxyq hoxyq Nov 21, 2025

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.

Does it have to be async, though? Does it fail if you check it synchronously?

If it fails, can you try emitting the event as part of act or actAsync?

utils.act(() => ownerWindow.dispatchEvent(keyEvent));


expect(store.profilerStore.isProfilingBasedOnUserInput).toBe(true);

// Dispatch keyboard event to toggle profiling off
ownerWindow.dispatchEvent(keyEvent);
await waitForProfilingState(false);

expect(store.profilerStore.isProfilingBasedOnUserInput).toBe(false);

document.body.removeChild(profilerContainer);
} finally {
// Restore fake timers for other tests
jest.useFakeTimers();
}

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.

Do you actually need this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Technically no because in the setupTest file the before each sets it to use fake timers. I guess the argument would be not being dependent on if that changes?

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.

You probably don't need fake timers here, because there is no logic that is tied to timers: intervals, timeouts, etc.

So if it passes with these lines removed, just remove it. If not, lets investigate why this is required.

@emily8rown emily8rown Nov 21, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

before when I used fake timers I was getting a time out after 5 seconds from the recursivelyflush in utils act and actAsync but this may just be because the profiler and its contexts have a lot of timers/ are more complex?

@emily8rown emily8rown Nov 21, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've got it to now use the fake timers and have turned recursively flush off to avoid the timeout

});
});
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
*/

import * as React from 'react';
import {Fragment, useContext} from 'react';
import {Fragment, useContext, useEffect, useRef, useEffectEvent} from 'react';
import {ModalDialog} from '../ModalDialog';
import {ProfilerContext} from './ProfilerContext';
import TabBar from '../TabBar';
Expand Down Expand Up @@ -38,6 +38,11 @@ import {TimelineContext} from 'react-devtools-timeline/src/TimelineContext';
import styles from './Profiler.css';

function Profiler(_: {}) {
const profilerRef = useRef<HTMLDivElement | null>(null);
const isMac =
typeof navigator !== 'undefined' &&
navigator.platform.toUpperCase().indexOf('MAC') >= 0;

const {
didRecordCommits,
isProcessingData,
Expand All @@ -47,6 +52,8 @@ function Profiler(_: {}) {
selectedTabID,
selectTab,
supportsProfiling,
startProfiling,
stopProfiling,
} = useContext(ProfilerContext);

const {file: timelineTraceEventData, searchInputContainerRef} =
Expand All @@ -56,6 +63,32 @@ function Profiler(_: {}) {

const isLegacyProfilerSelected = selectedTabID !== 'timeline';

// Cmd+E to start/stop profiler recording
const handleKeyDown = useEffectEvent((event: KeyboardEvent) => {
const correctModifier = isMac ? event.metaKey : event.ctrlKey;
if (correctModifier && event.key === 'e') {
if (isProfiling) {
stopProfiling();
} else {
startProfiling();
}
event.preventDefault();
event.stopPropagation();
}
});

useEffect(() => {
const div = profilerRef.current;
if (!div) {
return;
}
const ownerWindow = div.ownerDocument.defaultView;
ownerWindow.addEventListener('keydown', handleKeyDown);
return () => {
ownerWindow.removeEventListener('keydown', handleKeyDown);
};
}, []);

let view = null;
if (didRecordCommits || selectedTabID === 'timeline') {
switch (selectedTabID) {
Expand Down Expand Up @@ -112,7 +145,7 @@ function Profiler(_: {}) {

return (
<SettingsModalContextController>
<div className={styles.Profiler}>
<div ref={profilerRef} className={styles.Profiler}>
<div className={styles.LeftColumn}>
<div className={styles.Toolbar}>
<RecordToggle disabled={!supportsProfiling} />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,19 @@ export default function RecordToggle({disabled}: Props): React.Node {
className = styles.ActiveRecordToggle;
}

const isMac =
typeof navigator !== 'undefined' &&
navigator.platform.toUpperCase().indexOf('MAC') >= 0;
const shortcut = isMac ? '⌘E' : 'Ctrl+E';
const title = `${isProfiling ? 'Stop' : 'Start'} profiling - ${shortcut}`;

return (
<Button
className={className}
disabled={disabled}
onClick={isProfiling ? stopProfiling : startProfiling}
testName="ProfilerToggleButton"
title={isProfiling ? 'Stop profiling' : 'Start profiling'}>
title={title}>
<ButtonIcon type="record" />
</Button>
);
Expand Down
Loading