-
Notifications
You must be signed in to change notification settings - Fork 49.9k
[DevTools] hotkey to start/stop profiling #35160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DevTools] hotkey to start/stop profiling #35160
Conversation
packages/react-devtools-shared/src/devtools/views/Profiler/Profiler.js
Outdated
Show resolved
Hide resolved
packages/react-devtools-shared/src/devtools/views/Profiler/RecordToggle.js
Outdated
Show resolved
Hide resolved
packages/react-devtools-shared/src/devtools/views/Profiler/RecordToggle.js
Outdated
Show resolved
Hide resolved
packages/react-devtools-shared/src/devtools/views/Profiler/RecordToggle.js
Outdated
Show resolved
Hide resolved
packages/react-devtools-shared/src/devtools/views/Profiler/Profiler.js
Outdated
Show resolved
Hide resolved
| } finally { | ||
| // Restore fake timers for other tests | ||
| jest.useFakeTimers(); | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
packages/react-devtools-shared/src/__tests__/profilerContext-test.js
Outdated
Show resolved
Hide resolved
|
|
||
| try { | ||
| // Render the profiler - use React.act to ensure render completes | ||
| React.act(() => { |
There was a problem hiding this comment.
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.
|
|
||
| // Dispatch keyboard event to toggle profiling on | ||
| ownerWindow.dispatchEvent(keyEvent); | ||
| await waitForProfilingState(true); |
There was a problem hiding this comment.
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));
Co-authored-by: Ruslan Lesiutin <[email protected]>
…8rown/react into devtools-profiling-hot-key
## Summary The built-in browser profiler supports starting/stopping with Cmd+E. For Symmetry this adds the same hotkey for react devtools profiler. ## How did you test this change? yarn build:\<browser name\> yarn run test:\<browser name\> <img width="483" height="135" alt="Screenshot 2025-11-17 at 14 30 34" src="https://github.com/user-attachments/assets/426939aa-15da-4c21-87a4-e949e6949482" /> firefox: https://github.com/user-attachments/assets/6f225b90-828f-4e79-a364-59d6bc942f83 edge: https://github.com/user-attachments/assets/5b2e9242-f0e8-481b-99a2-2dd78099f3ac chrome: https://github.com/user-attachments/assets/790aab02-2867-4499-aec1-e32e38c763f9 --------- Co-authored-by: Ruslan Lesiutin <[email protected]> DiffTrain build for [fd524fe](fd524fe)
## Summary The built-in browser profiler supports starting/stopping with Cmd+E. For Symmetry this adds the same hotkey for react devtools profiler. ## How did you test this change? yarn build:\<browser name\> yarn run test:\<browser name\> <img width="483" height="135" alt="Screenshot 2025-11-17 at 14 30 34" src="https://github.com/user-attachments/assets/426939aa-15da-4c21-87a4-e949e6949482" /> firefox: https://github.com/user-attachments/assets/6f225b90-828f-4e79-a364-59d6bc942f83 edge: https://github.com/user-attachments/assets/5b2e9242-f0e8-481b-99a2-2dd78099f3ac chrome: https://github.com/user-attachments/assets/790aab02-2867-4499-aec1-e32e38c763f9 --------- Co-authored-by: Ruslan Lesiutin <[email protected]> DiffTrain build for [fd524fe](fd524fe)
Summary
The built-in browser profiler supports starting/stopping with Cmd+E. For Symmetry this adds the same hotkey for react devtools profiler.
How did you test this change?
yarn build:<browser name>
yarn run test:<browser name>
firefox:
https://github.com/user-attachments/assets/6f225b90-828f-4e79-a364-59d6bc942f83
edge:
https://github.com/user-attachments/assets/5b2e9242-f0e8-481b-99a2-2dd78099f3ac
chrome:
https://github.com/user-attachments/assets/790aab02-2867-4499-aec1-e32e38c763f9