[Devtools] Navigating commits performance panel hotkey #35238
Conversation
| selectedCommitIndex !== null | ||
| ) { | ||
| // Left/Right to navigate commits | ||
| if (event.key === 'ArrowLeft' || event.key === 'ArrowRight') { |
There was a problem hiding this comment.
We should probably check for Cmd key as well. I think we should keep just arrow navigations for changes between nodes in tree view (outside of the scope for this PR), and Cmd + ->, Cmd + <- for navigation between commits. What do you think?
There was a problem hiding this comment.
I agree it's nicer to have them unique but the arrow navigation for the tree view nodes only works when one has just been selected and when that has happened even having them as different shortcuts you can't use a shortcut to navigate between commits. I'm not convinced how much the tab navigation one will be used if your mouse has already selected a tab in the area there just before, and therefore it could be nicer to leave the shortcut as arrow key for simplicity
There was a problem hiding this comment.
Maybe it would confuse people they are switching tabs when they expect to switch commits if they don't realise they need to select it
There was a problem hiding this comment.
I am not sure I am following. We don't have keyboard navigation for the flamegraph in Profiler tree, right? But if we would add one, it would probably be done by using only arrow keys.
If so, then it would conflict with the navigation between commits. That's my current thinking, what is the alternative?
|
|
||
| // Profile and record multiple commits | ||
| await utils.actAsync(() => store.profilerStore.startProfiling()); | ||
| await utils.actAsync(() => legacyRender(<Parent />, container)); // Commit 1 |
There was a problem hiding this comment.
For new tests, we should try to use ReactDOMClient.createRoot and root.render(), unless we are testing against React with version prior to 18.
There was a problem hiding this comment.
Should there be a test for both?
There was a problem hiding this comment.
No, because this feature is independent from the version of React we are debugging.
| const numFilteredCommits = filteredCommitIndices.length; | ||
| if (selectedFilteredCommitIndex === null && numFilteredCommits > 0) { | ||
| selectCommitIndex(filteredCommitIndices[0]); | ||
| } else if ( | ||
| selectedFilteredCommitIndex !== null && | ||
| selectedFilteredCommitIndex >= numFilteredCommits | ||
| ) { | ||
| selectCommitIndex( | ||
| numFilteredCommits === 0 | ||
| ? null | ||
| : filteredCommitIndices[numFilteredCommits - 1], | ||
| ); | ||
| } |
There was a problem hiding this comment.
Can we move this to setMinCommitDuration? So that these updates get batched and to avoid cascading update here, even if it could be cheap
| document.body.removeChild(profilerContainer); | ||
| }); | ||
|
|
||
| // @reactVersion >= 18 |
There was a problem hiding this comment.
No need to specify this. If you remove the pragma, it will only run the test against React from source.
…ance-panel-hot-key
https://github.com/emily8rown/react into devtools-navigating-commits-performance-panel-hot-key
| [setRootID, selectFiber], | ||
| ); | ||
|
|
||
| // why am i doing this instead of use effect |
There was a problem hiding this comment.
In most of the cases, passive effects (useEffect(...)) run after the new UI is painted on the screen. If this logic would be inside an effect, then the browser could paint a frame with a UI derived from a stale state. This is quite common root cause of UI thrashing problems.
Having this logic inside a render function causes a cascading re-render, but it eliminates the possibility of showing a stale UI that will be changed by the next frame.
2 participants
Summary
Add keyboard shortcuts (Cmd/Ctrl + Left/Right arrow keys) to navigate between commits in the Profiler's snapshot view.
Moved
filteredCommitIndicesmanagement and commit navigation logic (selectNextCommitIndex,selectPrevCommitIndex) fromSnapshotSelectorintouseCommitFilteringAndNavigationused byProfilerContextto enable keyboard shortcuts from the top-level Profiler component.How did you test this change?
yarn build:<browser name>yarn run test:<browser name>Chrome:
Screen.Recording.2025-11-27.at.18.19.45.mov
Edge with duration filter:
Screen.Recording.2025-11-28.at.12.23.23.mov
firefox mixing hotkey with clicking arrow buttons:
Screen.Recording.2025-11-28.at.12.34.07.mov