Add context menu option for opening source view (Fixes #4405)

[krsh732]

Hey, it seems to look fine visually and functionally as far as I tested, so hopefully it's not horribly wrong:

Instead of restricting the context menu option to flame graph and call tree tabs only, I also allowed stack chart to open the source view via right-click and enter. Didn't do double click in stack chart though, since it's used for something else. Is this fine?

Also, is there anything more I need to be doing for i18n/l10n during or if/after this PR lands?

Fixes #4405

Production
Deploy preview

[codecov]

Codecov Report

Base: 88.52% // Head: 88.58% // Increases project coverage by +0.05% 🎉

Coverage data is based on head (731ae7f) compared to base (b6bba99).
Patch coverage: 87.80% of modified lines in pull request are covered.

❗ Current head 731ae7f differs from pull request most recent head 07344f4. Consider uploading reports for the commit 07344f4 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4423      +/-   ##
==========================================
+ Coverage   88.52%   88.58%   +0.05%     
==========================================
  Files         283      282       -1     
  Lines       25508    25518      +10     
  Branches     6869     6875       +6     
==========================================
+ Hits        22582    22606      +24     
+ Misses       2717     2707      -10     
+ Partials      209      205       -4     

Impacted Files	Coverage Δ
src/components/shared/TreeView.js	82.55% <33.33%> (+0.10%)	⬆️
src/components/flame-graph/FlameGraph.js	78.43% <87.50%> (+3.92%)	⬆️
src/components/shared/CallNodeContextMenu.js	90.24% <100.00%> (+0.72%)	⬆️
src/components/stack-chart/index.js	86.88% <100.00%> (+1.17%)	⬆️
src/selectors/profile.js	95.66% <0.00%> (ø)
src/utils/react.js
src/components/timeline/TrackPowerGraph.js	93.47% <0.00%> (+0.47%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[mstange]

This looks great! Adding Julien and Nazim so that they can comment on whether they agree with this UI and implementation.

I wonder if it's worth adding a tooltip to this context menu item. The transform items have tooltips, but that's because they really need tooltips, heh. It's probably fine to have no tooltip for this new item.

I did notice one problem in the deploy preview, but this is totally my fault and a problem with the initial implementation, and we can fix it in a follow-up. I made the assumption that the call node which opens the source view is always the selected call node. But when using the context menu to open the source view, the right clicked call node can be different from the selected call node. This is a problem because the selected call node influences the behavior of the source view in that it determines which line is scrolled into view. You can see this in this profile in the deploy preview as follows:

1. Select the call node with the function "js::gc::GCRuntime::gcSlice".
2. Right click the call node with the function "js::gc::GCRuntime::markUntilBudgetExhausted" (6 frames deeper than the selected call node). Both these functions are located in GC.cpp.
3. Click "Show GC.cpp".

Now the source view will show GC.cpp, but it will be scrolled to display the contents of the "js::gc::GCRuntime::gcSlice" function. Ideally, it would be scrolled to display the contents of the "markUntilBudgetExhausted" function. Fixing this is probably a bit complicated, so let's leave it for a follow-up.

[krsh732]

I wonder if it's worth adding a tooltip to this context menu item.

I wouldn't mind adding a tooltip, it's just that I don't know what the tooltip should be saying 😛

Also, should I be adding tests for this since coverage is dropping? I tried to add a test to CallNodeContextMenu.test.js that looks like this:

it('can show source file', function () {
  const {
    profile,
    funcNamesPerThread: [funcNames],
  } = getProfileFromTextSamples(`
    A[file:git:github.com/rust-lang/rust:library/std/src/sys/unix/thread.rs:53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b]
  `);
  const funcIndex = funcNames.indexOf('A');

  const store = storeWithProfile(profile);
  store.dispatch(changeRightClickedCallNode(0, [funcIndex]));
  const { getByText } = setup(store);

  expect(document.querySelector(".sourceView")).not.toBeInTheDocument();
  fireFullClick(getByText(/Show/));
  expect(document.querySelector(".sourceView")).toBeInTheDocument();
});

But it didn't really work out since setup only seems to render the CallNodeContextMenu component, so the source view is never in the document. Is there an easy way to fix this test, or should I instead be writing a test in SourceView.test.js similar to "should show the source view when double clicking on a line in the tree view"?

I did notice one problem in the deploy preview, ...

Similarly, there is an inconsistency with keyboard shortcuts in the context menu. Right-clicking a different call node from the selected node and using any of the call node transform shortcuts, results on the transform being applied to the right-clicked node. However, when pressing enter in this situation, it shows the source view for the selected call node instead. Furthermore, the call node transform shortcuts seem to dismiss the context menu, while other shortcuts such as "Expand all" and "Show file" don't. Should this also be tackled in the follow up?

Regarding the problem itself, I saw that BottomBox.js is using selectedCallNodeTimings. So I was thinking of:

1. Sending line timings as part of the open source view action.
2. Making BottomBox get the timings from the state similar to how it gets the file path.
3. Make BottomBox then pass these timings to the SourceView component instead.

However, I don't know how to get the line timings for the right clicked call node without potentially copy-pasting and slightly modifying a whole bunch of code from per-thread/index.js (also not sure if it'll work after doing this). So maybe I can watch and learn from whoever does the follow up 😄

[julienw]

Similarly, there is an inconsistency with keyboard shortcuts in the context menu. Right-clicking a different call node from the selected node and using any of the call node transform shortcuts, results on the transform being applied to the right-clicked node. However, when pressing enter in this situation, it shows the source view for the selected call node instead.

Yeah it would be good to fix this, but I even think we should try to fix this before merging this patch actually. The alternative is that we don't specify the "Enter" shortcut for now.

Furthermore, the call node transform shortcuts seem to dismiss the context menu, while other shortcuts such as "Expand all" and "Show file" don't. Should this also be tackled in the follow up?

I don't see this on my side.

[julienw]

Regarding the problem itself, I saw that BottomBox.js is using selectedCallNodeTimings. So I was thinking of:

1. Sending line timings as part of the open source view action.

2. Making BottomBox get the timings from the state similar to how it gets the file path.

3. Make BottomBox then pass these timings to the SourceView component instead.

However, I don't know how to get the line timings for the right clicked call node without potentially copy-pasting and slightly modifying a whole bunch of code from per-thread/index.js (also not sure if it'll work after doing this). So maybe I can watch and learn from whoever does the follow up smile

The plan sounds good to me at least :-)

[julienw]

But it didn't really work out since setup only seems to render the CallNodeContextMenu component, so the source view is never in the document. Is there an easy way to fix this test, or should I instead be writing a test in SourceView.test.js similar to "should show the source view when double clicking on a line in the tree view"?

I think it would be good enough to check the content of UrlState.sourceView in this case.

[julienw]

This looks reasonable, thanks for the patch!

As you mentioned, it would be good to have some tests for the new behaviors (menu + stack chart). That's why I'm marking 'Request Changes'.

Thanks again!

[julienw]

Fixing the Enter key shouldn't be too hard:

• in TreeView, stop providing the nodeId when calling onEnterKey and onDoubleClick props
• in CallTree, get the information like we do for handleTransformKey:

    const nodeIndex =
      rightClickedCallNodeIndex !== null
        ? rightClickedCallNodeIndex
        : selectedCallNodeIndex;

[krsh732]

Sorry for the delayed response, still at work right now, but hopefully I'll have addressed some of these later tonight.

Furthermore, the call node transform shortcuts seem to dismiss the context menu, while other shortcuts such as "Expand all" and "Show file" don't. Should this also be tackled in the follow up?

I don't see this on my side.

Here's what I observe on production:

• Focus on function: Dismisses context menu upon using shortcut.
• Expand all: Doesn't dismiss context menu upon using shortcut.

Fixing the Enter key shouldn't be too hard

If we fix the Enter key should we be fixing "*" (Expand all) too?
Also, it gets a bit more confusing when taking into context that there is a bug relating to context menu focus:

• Call Tree: Arrow keys are controlling the call tree instead of the context menu.
• Flame Graph: Arrow keys are controlling both the canvas and context menu.
• Stack Chart: Arrow keys are only controlling the context menu (probably coincidence, since stack chart doesn't do anything with arrow keys)

To me, the stack chart like behavior makes most sense. The context menu has focus, so keys should only be controlling that and not things behind it. However, if this behavior is made consistent, the Enter key when pressed in a context menu on options other than "Show file" would be selecting that option rather than showing the file. In contrast, using the other shortcuts regardless of which context menu option is selected, would trigger the action for the shortcut. Would that be acceptable or should we not show the enter shortcut in the context menu? Also apologies, I can't tell if I'm over-complicating/thinking things 😄

I think it would be good enough to check the content of UrlState.sourceView in this case.

Awesome, I think I can write a test asserting against the sourceViewFile.

[krsh732]

Fixing the Enter key shouldn't be too hard:

* in TreeView, stop providing the nodeId when calling `onEnterKey` and `onDoubleClick` props

* in CallTree, get the information like we do for `handleTransformKey`:

    const nodeIndex =
      rightClickedCallNodeIndex !== null
        ? rightClickedCallNodeIndex
        : selectedCallNodeIndex;

I've done this a bit differently from what was mentioned here because I also wanted to fix the expand all shortcut for consistency, hopefully it's OK and I haven't broken anything.

it would be good to have some tests for the new behaviors (menu + stack chart)

I've also added some tests for the Enter key on stack chart and "Show file" context menu option. The project coverage checks now pass, but the patch ones don't. Feel free to let me know if I should be writing additional tests or if this is OK.

I wrote earlier:
... Enter key when pressed in a context menu on options other than "Show file" would be selecting that option rather than showing the file. In contrast, using the other shortcuts regardless of which context menu option is selected, would trigger the action for the shortcut.

I think showing the Enter key as a shortcut despite this is fine, since other programs seem to do it too. For example, windows explorer:

Outstanding things that were noticed while working on this PR but not addressed by the PR:

1. Source view doesn't seek to the right line in file when right clicked node is different from selected node
2. Keyboard shortcuts affect varying nodes in varying tabs (eg. arrow keys in call tree affect call tree only while arrow keys in flame graph affects both flame graph and context menu)
3. Some keyboard shortcuts dismiss the context menu (ie. call node transforms) while others don't (ie. expand all and the show source option added in this PR)

Feel free to let me know if I should be filing any of these as issues or tackling them in this PR itself.

[julienw]

This looks pretty solid to me! Indeed your solution to fix Enter and * looks just as good as mine.

It would be good to have a test for the flame graph changes. You can shamelessly plug into the existing test can be navigated with the keyboard. The function J already has a file name attached to it (look at setupFlameGraph), maybe you can attach one to B as well so that you don't have to target J specifically in that test.

It would also be good to have a test for "*" and "Enter" in a right click setup but I don't think there is a prior existing test you could reuse, so I won't ask you to provide one.

[julienw]

Outstanding things that were noticed while working on this PR but not addressed by the PR:

1. Source view doesn't seek to the right line in file when right clicked node is different from selected node

2. Keyboard shortcuts affect varying nodes in varying tabs (eg. arrow keys in call tree affect call tree only while arrow keys in flame graph affects both flame graph and context menu)

3. Some keyboard shortcuts dismiss the context menu (ie. call node transforms) while others don't (ie. expand all and the show source option added in this PR)

Feel free to let me know if I should be filing any of these as issues or tackling them in this PR itself.

I think it would be better to file them separately. We prefer smaller targeted PRs as they're easier to review :-)
Thanks again!

[krsh732]

It would be good to have a test for the flame graph changes.

Added to can be navigated with the keyboard as you mentioned.

[julienw]

Looks good to me, thanks a lot!

We're still waiting for the l10n approval before merging.