Add Component Highlighting to Profiler DevTools #17934

M-Izadmehr · 2020-01-30T03:58:52Z

Summary: Fix #17928

Inside the devTools (Components) we have component highlighting, and by hovering the mouse on components it will highlight them in DOM. However, this feature was missing from Profiler (both FlameGraph Chart and Ranked Chart). The purpose of this PR is to add this functionality to the profiler.

Demo

codesandbox-ci · 2020-01-30T04:01:08Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ce9b3a8:

Sandbox	Source
holy-tree-1kvll	Configuration

sizebot · 2020-01-30T04:04:06Z

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against ce9b3a8

sizebot · 2020-01-30T04:04:22Z

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against ce9b3a8

strdr4605 · 2020-01-30T08:54:34Z

packages/react-devtools-shared/src/devtools/views/Profiler/CommitFlamegraph.js

+  const highlightNativeElement = useCallback(
+    (id: number) => {
+      const element = store.getElementByID(id);
+      const rendererID = store.getRendererIDForElement(id);
+      if (element !== null && rendererID !== null) {
+        bridge.send('highlightNativeElement', {
+          displayName: element.displayName,
+          hideAfterTimeout: false,
+          id,
+          openNativeElementsPanel: false,
+          rendererID,
+          scrollIntoView: false,
+        });
+      }
+    },
+    [store, bridge],
+  );
+
+  // Highlight last hovered element.
+  const handleElementMouseEnter = useCallback(
+    id => {
+      highlightNativeElement(id);
+    },
+    [highlightNativeElement],
+  );
+


This section is also repeated in packages/react-devtools-shared/src/devtools/views/Profiler/CommitRanked.js and also in
packages/react-devtools-shared/src/devtools/views/Components/Tree.js.

Maybe better to create a separate hook file?

Thanks for the comment, Yeah I absolutely agree, I will try to use a hook for the highlighting functionality:)

Done, now I extracted the highlighting logic to the hooks file, and created a hook called useNativeElementHighlighter, this hook exposes two methods:

highlightNativeElement: method used to highlight an element using its id

clearNativeElementHighlight: method used to clear highlighting of elements

strdr4605 · 2020-01-30T13:17:24Z

packages/react-devtools-shared/src/devtools/views/Profiler/CommitFlamegraph.js

+  );
+
+  // Highlight last hovered element.
+  const handleElementMouseEnter = useCallback(


@M-Izadmehr, can you please explain why you wrap in another useCallback?

Done:)
I removed the useless useCallback.

bvaughn · 2020-01-30T18:26:40Z

I'm going to hold off doing a review for now since it sounds like there are planned changes. 😄

M-Izadmehr · 2020-02-02T18:10:32Z

@bvaughn As a response to the reviews I made the following changes:

I extracted the highlighting logic to the hooks file, and created a hook called useNativeElementHighlighter, which exposes two methods (highlightNativeElement and clearNativeElementHighlight).
Added clear highlighting when you leave the element
Added a logic when leaving an element, if another element is already selected use highlighting on that (see the gif)

I guess it is now in a good state to be merged:))

bvaughn · 2020-02-02T20:09:16Z

Might be worth profiling the profiler (in the test shell) before and after this change to see if we added any extra renders or otherwise did anything that would cause a regression in the flame chart views.

I'll review this sometime in the next couple of days though~

bvaughn · 2020-02-04T16:52:27Z

Might be worth profiling the profiler (in the test shell) before and after this change to see if we added any extra renders or otherwise did anything that would cause a regression in the flame chart views.

Any chance you were able to do this? 😁

bvaughn · 2020-02-04T19:14:51Z

I did a quick test. I don't see any big perf regressions, but I do see a UX problem: Highlights always get stuck. Looks like anytime I'm not moused over the root flamegraph node, its DOM element is highlighted - and mousing out of the graph doesn't clear the highlight either, leaving it stuck on the page.

M-Izadmehr · 2020-02-05T00:10:01Z

Thanks for the comment :)
Yeah, I think that was a bad decision on my side because I thought it might be a good idea if a fiber is already selected and the user is not hovering an element, highlight the clicked fiber. However, it results in some bad implications:

If the user redirects to a new page (where the fiber does not exist anymore) the highlight still remains on the page.
Also, in this way, we should keep track of valid fibers

As a result, I removed that unnecessary logic (ao that logic will be similar to components devtools).

packages/react-devtools-shared/src/devtools/views/hooks.js

packages/react-devtools-shared/src/devtools/views/Components/Tree.js

packages/react-devtools-shared/src/devtools/views/Profiler/CommitFlamegraph.js

bvaughn · 2020-02-18T17:49:10Z

packages/react-devtools-shared/src/devtools/views/Profiler/CommitFlamegraph.js

@@ -115,9 +122,21 @@ function CommitFlamegraph({chartData, commitTree, height, width}: Props) {
    return null;
  }, [chartData, selectedFiberID, selectedChartNodeIndex]);

+  // Highlight last hovered element.
+  const handleElementMouseEnter = id => {


Unnecessary function wrapper. I'd suggest just passing highlightNativeElement directly (or if you think the naming is clearer, just alias, e.g. const handleElementMouseEnter = highlightNativeElement )

After merging the other PR, this method now does two actions:

Adding the highlighting,

Setting the fiberData used for the tooltip,
So, I guess we can keep the wrapper.

bvaughn · 2020-02-18T17:50:48Z

packages/react-devtools-shared/src/devtools/views/Profiler/CommitRanked.js


  const selectedFiberIndex = useMemo(
    () => getNodeIndex(chartData, selectedFiberID),
    [chartData, selectedFiberID],
  );

+  // Highlight last hovered element.
+  const handleElementMouseEnter = id => {


Same as above comment.

Same as above comment:)

packages/react-devtools-shared/src/devtools/views/Profiler/CommitRanked.js

packages/react-devtools-shared/src/devtools/views/hooks.js

M-Izadmehr · 2020-02-19T11:57:12Z

Thanks for the comments,
I think, as two pull requests will have minor conflicts with each other as both are working on hovering the fiber and adding some custom event listeners to mouse, I think it might be better to first merge one of them and afterward I will apply the reviews to this one (as some of the changes are already fixed in the other one).

bvaughn · 2020-02-19T18:40:31Z

Sounds good @M-Izadmehr. The other PR has been merged!

bvaughn · 2020-02-27T16:43:27Z

@M-Izadmehr Still planning to update this PR? Or would you like me to finish it up?

M-Izadmehr · 2020-02-27T20:55:47Z

@bvaughn Sorry last week was super busy with some releases, I actually wanted to do it in the weekend, does it sound good?:)

bvaughn · 2020-02-27T20:56:46Z

Sure sounds fine.

M-Izadmehr · 2020-03-01T21:13:54Z

Sorry for the late update, I merged the other stuff to this PR, now it seems stable, I don't see any additional re-renders:)

Here is a demo with both tooltip and highlighting:

bvaughn

While reviewing this, I realized that if you load exported profiling data from a previous session, it would end up highlighting arbitrary components on the page you're currently viewing (since ids are just auto-incremented numbers). We could use actual GUIDs- less likely to conflict, but I don't really want to add the extra overhead. Maybe instead we should track whether the profiler data was imported or recorded live, and...disable the hover feature for imported data.

I don't know. I might want to think on that a little, although if you have any ideas I"d be open to discussing them 😄

bvaughn · 2020-03-02T21:32:36Z

packages/react-devtools-shared/src/devtools/views/Profiler/CommitFlamegraph.js

@@ -96,9 +99,13 @@ type Props = {|
 |};

 function CommitFlamegraph({chartData, commitTree, height, width}: Props) {
-  const [hoveredFiberData, hoverFiber] = useState<number | null>(null);
+  const [hoveredFiberData, setHoveredFiberData] = useState<number | null>(null);


This Flow type looks wrong. (Looks like it's been wrong for a while though FWIW- which is a bit concerning.)

We aren't storing a number in this state, but an object with id and name props (TooltipFiberData).

bvaughn · 2020-03-02T21:32:55Z

packages/react-devtools-shared/src/devtools/views/Profiler/CommitFlamegraph.js

  const itemData = useMemo<ItemData>(
    () => ({
      chartData,
-      hoverFiber,
+      isHovered: hoveredFiberData && hoveredFiberData.id,


Why is this check not just hoveredFiberData !== null?

bvaughn · 2020-03-02T21:35:07Z

packages/react-devtools-shared/src/devtools/views/Profiler/CommitFlamegraph.js

@@ -140,7 +159,7 @@ function CommitFlamegraph({chartData, commitTree, height, width}: Props) {
    }),
    [
      chartData,
-      hoverFiber,
+      setHoveredFiberData,


This looks wrong. The dependencies array has setHoveredFiberData but the memoized value references hoveredFiberData.

I wonder why our lint rule isn't complaining about this.

bvaughn · 2020-03-02T21:35:34Z

packages/react-devtools-shared/src/devtools/views/Profiler/CommitFlamegraph.js

-      hoverFiber,
+      isHovered: hoveredFiberData && hoveredFiberData.id,
+      onElementMouseEnter: handleElementMouseEnter,
+      onElementMouseLeave: handleElementMouseLeave,


This looks wrong. Neither handleElementMouseEnter nor handleElementMouseLeave are mentioned in the dependencies array.

I wonder why our lint rule isn't complaining about this.

bvaughn · 2020-03-02T21:39:47Z

packages/react-devtools-shared/src/devtools/views/Profiler/CommitRanked.js

@@ -94,19 +97,35 @@ type Props = {|
 |};

 function CommitRanked({chartData, commitTree, height, width}: Props) {
-  const [hoveredFiberData, hoverFiber] = useState<number | null>(null);
+  const [hoveredFiberData, setHoveredFiberData] = useState<number | null>(null);


Same comment as above wrt hook state type.

bvaughn · 2020-03-02T21:40:04Z

packages/react-devtools-shared/src/devtools/views/Profiler/CommitRanked.js

-      hoverFiber,
+      isHovered: hoveredFiberData && hoveredFiberData.id,
+      onElementMouseEnter: handleElementMouseEnter,
+      onElementMouseLeave: handleElementMouseLeave,


Same comment as above wrt missing dependencies.

bvaughn · 2020-03-02T21:40:15Z

packages/react-devtools-shared/src/devtools/views/Profiler/CommitRanked.js

  const itemData = useMemo<ItemData>(
    () => ({
      chartData,
-      hoverFiber,
+      isHovered: hoveredFiberData && hoveredFiberData.id,


Same comment as above wrt hoveredFiberData !== null

bvaughn · 2020-03-03T19:06:45Z

Some follow up from my review yesterday.

Turns out our Flow coverage was really broken for DevTools. Oops! Fixed with #18204.

The "rules of hooks" lint rule isn't being run on this repo, but should be. I'll look to add it with a separate PR.

M-Izadmehr · 2020-03-03T22:03:50Z

Oh I see, I can also try to check it out if we are using it on all the packages

M-Izadmehr · 2020-03-03T22:08:11Z

@bvaughn Yeah, you are right, we can check if the element is imported, I think in the tooltip we already have information on that part (only if we are listening for changes for that), we can re-use the logic there for the highlighter too.
One simple way without performance reduction is to only enable highlighter, only when why rendered is true.
I will play around with it to see if there is any optimal solution for this:)

gaearon · 2020-04-01T19:09:56Z

@M-Izadmehr Would be great if we could get this landed. :-)

M-Izadmehr · 2020-04-01T22:08:12Z

@gaearon Yeah, please don't close this one, I was really busy in the past month and couldn't come back to this issue, I will try to pick it up in the weekend (thanks to quarantine, we have so much free time now:) )

bvaughn · 2020-04-03T17:43:55Z

Hey @M-Izadmehr, PR #18479 is is going to cause a bit of a conflict for you b'c it also separates the highlight behavior into a hook. Looks like the hook is the same as yours though, so it should be easy for you to rebase and just use it instead. Just wanted to give you a heads up!

gaearon · 2020-04-09T20:36:18Z

@M-Izadmehr Friendly ping :-) Do you think you might be able to get this over the finish line?

bl00mber · 2020-06-18T16:31:27Z

Hey @M-Izadmehr can you please explain how you managed to request a review and not be a member at the same time? 🤔

M-Izadmehr · 2020-06-19T13:18:29Z

@bl00mber when you are finished with your stuff, you can ask @bvaughn to check it out (as it is related to DevTools, and he is the DevTools god:) )

bvaughn · 2020-06-19T19:43:23Z

@bl00mber I've reviewed several of your PRs in the past month or so 😄

It's important to keep in mind that there are only a half dozen or so React core members trying to keep up with PRs from > 1,000 contributors (while at the same time creating our own as well).

bl00mber · 2020-06-19T20:23:10Z

@bvaughn yeah thanks, I've actually do not wanted to request a review now (may be only on stalled PRs), was just curious how the author of this PR is able to do it.

stale · 2020-09-20T04:03:29Z

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

stale · 2020-10-04T06:19:53Z

Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you!

M-Izadmehr added 4 commits January 30, 2020 04:12

Add highlighting to flame graph

fe43dc8

Add on mouse enter to flame graph props

0deb555

Add highlighting to commit ranked profiler

53a84ce

Fix linting

d539137

facebook-github-bot added the CLA Signed label Jan 30, 2020

M-Izadmehr mentioned this pull request Jan 30, 2020

Profiler should highlight host components (e.g. DOM elements) on mouseover #17928

Closed

M-Izadmehr requested a review from bvaughn January 30, 2020 04:02

strdr4605 suggested changes Jan 30, 2020

View reviewed changes

strdr4605 reviewed Jan 30, 2020

View reviewed changes

bvaughn self-assigned this Jan 30, 2020

jameskip approved these changes Jan 30, 2020

View reviewed changes

M-Izadmehr added 5 commits February 2, 2020 18:29

Create hooks for devtools add and clear highlighting

5f66d47

Use hooks for highlighting in devtools tree

c6bcac3

Use element highlighter hooks in CommitRanked

ed6fe06

Use element highlighter hooks in FlameGraph devtools

c73bd21

Devtools: in Profiler, on mouse leave highlight the clicked element

49ea156

Fix linting

ab5b537

strdr4605 approved these changes Feb 2, 2020

View reviewed changes

Add onMouseLeave flow types

0fd63c0

Remove onMouse click highlighting

2958df7

bvaughn reviewed Feb 5, 2020

View reviewed changes

packages/react-devtools-shared/src/devtools/views/hooks.js Show resolved Hide resolved

ermanzohre approved these changes Feb 18, 2020

View reviewed changes

bvaughn reviewed Feb 18, 2020

View reviewed changes

M-Izadmehr added 3 commits March 1, 2020 21:32

Merge master to highlighting

0a24f63

Fix PR comments

1c1c2b6

Merge branch 'master' into profiler-highlighting

ce9b3a8

bvaughn reviewed Mar 2, 2020

View reviewed changes

bvaughn mentioned this pull request Mar 3, 2020

Fix flow types #18204

Merged

bvaughn mentioned this pull request Apr 3, 2020

[DevTools] - Highlight rendered by elements on hover. #18479

Merged

bl00mber mentioned this pull request Apr 26, 2020

[DevTools] Add Component Highlighting to Profiler #18745

Merged

stale bot added the Resolution: Stale Automatically closed due to inactivity label Sep 20, 2020

stale bot closed this Oct 4, 2020

Add Component Highlighting to Profiler DevTools #17934

Add Component Highlighting to Profiler DevTools #17934

Conversation

M-Izadmehr commented Jan 30, 2020

Summary: Fix #17928

Demo

codesandbox-ci bot commented Jan 30, 2020 • edited Loading

sizebot commented Jan 30, 2020 • edited Loading

Size changes (stable)

sizebot commented Jan 30, 2020 • edited Loading

Size changes (experimental)

strdr4605 Jan 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn commented Jan 30, 2020

M-Izadmehr commented Feb 2, 2020 • edited Loading

bvaughn commented Feb 2, 2020

bvaughn commented Feb 4, 2020

bvaughn commented Feb 4, 2020

M-Izadmehr commented Feb 5, 2020 • edited Loading

Choose a reason for hiding this comment

M-Izadmehr Mar 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M-Izadmehr commented Feb 19, 2020

bvaughn commented Feb 19, 2020

bvaughn commented Feb 27, 2020

M-Izadmehr commented Feb 27, 2020

bvaughn commented Feb 27, 2020

M-Izadmehr commented Mar 1, 2020

bvaughn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn commented Mar 3, 2020

M-Izadmehr commented Mar 3, 2020

M-Izadmehr commented Mar 3, 2020

gaearon commented Apr 1, 2020

M-Izadmehr commented Apr 1, 2020

bvaughn commented Apr 3, 2020

gaearon commented Apr 9, 2020

bl00mber commented Jun 18, 2020

M-Izadmehr commented Jun 19, 2020

bvaughn commented Jun 19, 2020

bl00mber commented Jun 19, 2020

stale bot commented Sep 20, 2020

stale bot commented Oct 4, 2020

codesandbox-ci bot commented Jan 30, 2020 •

edited

Loading

sizebot commented Jan 30, 2020 •

edited

Loading

sizebot commented Jan 30, 2020 •

edited

Loading

strdr4605 Jan 30, 2020 •

edited

Loading

M-Izadmehr commented Feb 2, 2020 •

edited

Loading

M-Izadmehr commented Feb 5, 2020 •

edited

Loading

M-Izadmehr Mar 1, 2020 •

edited

Loading