1442884 - "Show request details" button does not work when the first request list item is not shown using any filters

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[magicp]

Attachment: steps-to-reproduce_20180303_235253.mp4

Steps to reproduce:
1. Launch Nightly
2. Go to any sites (e.g. https://www.mozilla.org/en-US/contribute/)
3. Open Network monitor (Ctrl+Shift+E)
4. Reload the page
5. Apply "JS" filter (Hide the first request list item using filter) 
6. Select any request list item (Detail panel is shown)
7. Click "Hide request details" button
8. Click "Show request details" button

Actual results:
"Show request details" button does not work when the first request list item is not shown using any filters. Because "Show request details" button always show the first request item.

Expected results:
"Show request details" shows the last selected request list item. If there is no selected request list item, it shows the first one in current list.

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I can reproduce the problem on my machine.

Thanks for the report Tim!

Honza

Comment 2

[neoco]

Hi, I'm a computer science student at University of Toronto and one of my courses involves fixing a bug. I have decided to choose this bug as one of my first bugs. I am attempting to find the file that contains the buggy code. I suspect that it is in one of the files in charge of the network monitor GUI. Are you able to provide any pointers on where the GUI files for the network monitor would be located? Thanks

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to neoco from comment #2)
> Hi, I'm a computer science student at University of Toronto and one of my
> courses involves fixing a bug. I have decided to choose this bug as one of
> my first bugs. I am attempting to find the file that contains the buggy
> code. I suspect that it is in one of the files in charge of the network
> monitor GUI. Are you able to provide any pointers on where the GUI files for
> the network monitor would be located? Thanks

Sounds great, do you have an experience with React/Redux?

Honza

Comment 4

[neoco]

(In reply to Jan Honza Odvarko [:Honza] from comment #3)
> (In reply to neoco from comment #2)
> > Hi, I'm a computer science student at University of Toronto and one of my
> > courses involves fixing a bug. I have decided to choose this bug as one of
> > my first bugs. I am attempting to find the file that contains the buggy
> > code. I suspect that it is in one of the files in charge of the network
> > monitor GUI. Are you able to provide any pointers on where the GUI files for
> > the network monitor would be located? Thanks
> 
> Sounds great, do you have an experience with React/Redux?
> 
> Honza

Hi, sorry for the late response. I am familiar with React but not with Redux

I've looked into this further and I suspect that the problem is in mozilla-central/devtools/client/netmonitor/src/components/Toolbar.js. It looks like there is a function called dispatch that may be related to this bug

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I've done some analysis and here are detailed comments:

1) The details panel is represented by NetworkDetailsPanel component (implemented in components/NetworkDetailsPanel.js file)

2) This component is being rendered by MonitorPanel component when `networkDetailsOpen` props is true.

3) `networkDetailsOpen` is set according to `state.ui.networkDetailsOpen` field that is defined in `reducers/ui` reducer.

4) The state (`networkDetailsOpen`) can be changed by firing OPEN_NETWORK_DETAILS action - this can be done through `openNetworkDetails` creator defined in `actions/ui`

5) There is also `toggleNetworkDetails` action creator implemented on top of `openNetworkDetails`. This action simple flips the `networkDetailsOpen` state and is directly used by `components/Toolbar` component when the user clicks the `hide/show request details button`.

6) Important thing is also implementation of `componentDidUpdate` method in MonitorPanel component. It's checking `selectedRequestVisible` property and calling `openNetworkDetails(false);` if the prop is false. In other words, if the selected request is NOT visible -> do not show/render the NetworkDetailsPanel component. This property is calculated using `isSelectedRequestVisible(state),` selector in Redux connect method (at the bottom of the MonitorPanel.js file)

7) `isSelectedRequestVisible` is implemented in `selectors/requests.js` file. The implementation is using `getDisplayedRequests` method to get all displayed requests and checking whether `selectedId` (the current selected request ID) is in the list.

8) `selectedId` is calculated in `reducers/requests` reducer, search for:
nextState.selectedId = [...state.requests.values()][0].id;
It's using the first request by default - from `state.request` that represents ALL requests, NOT the displayed ones. And in our case the first request is not visible. So `selectedId` refers to a request that is not visible and that's why `isSelectedRequestVisible` return false and MonitorPanel component calls `openNetworkDetails(false);` -> I.e. the NetworkDetailsPanel component is immediately hidden.

9) So, my feeling is that we need to fix this line:
nextState.selectedId = [...state.requests.values()][0].id;
to make sure that first item from list of DISPLAYED requests is used instead. Something like:

let requests = getDisplayedRequests(state);
nextState.selectedId = requests.length ? requests[0].id : null;

But, I am afraid that `getDisplayedRequests` can't be used in the context of the `requests` reducer. Anyway this feels like the place that needs attention.

---

I think it could be relatively simple to fix for someone who has React/Redux experience; otherwise it could be too complex. 


Honza

Comment 6

[neoco]

Thanks for the very detailed analysis; it really helped me understand why the bug was happening. Unfortunately, I don't think I can fix the bug because I don't know enough about React/Redux. The bug is way too complex for me. I apologize for any inconvenience I have caused in failing to fix this bug

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to neoco from comment #6)
> Thanks for the very detailed analysis; it really helped me understand why
> the bug was happening. Unfortunately, I don't think I can fix the bug
> because I don't know enough about React/Redux. The bug is way too complex
> for me. I apologize for any inconvenience I have caused in failing to fix
> this bug
No problem at all!

Honza

Comment 8

[mayank9856]

Attachment: Bug 1442884 - Show request details button does not work when the first request list item is not shown using any filters.

Review commit: https://reviewboard.mozilla.org/r/232940/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/232940/

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8964195 [details]
Bug 1442884 - Show request details button does not work when the first request list item is not shown using any filters.

https://reviewboard.mozilla.org/r/232940/#review238432

Thanks for working on this!
I've been playing with the patch and it fixed the reported issue.

But, please see my inline comments.

We also need a test for this.

All existings tests are in nemonitor/tests dir

You might tage a look at browser_net_pane-toggle.js to get some inspiration.

Honza

::: commit-message-ef717:1
(Diff revision 1)
> +Bug 1442884 - Show request details button does not work when the first request list item is not shown using any filters. r:Honza

You need to use question mark in the commit message:

r?Honza

(notice that there is no reviewer set for the patch atm)

::: devtools/client/netmonitor/src/actions/ui.js:20
(Diff revision 1)
>    SELECT_DETAILS_PANEL_TAB,
>    TOGGLE_COLUMN,
>    WATERFALL_RESIZE,
>  } = require("../constants");
>  
> +const {getDisplayedRequests} = require("../selectors/index");

nit: there should be spaces as follows:

`{ getDisplayedRequests }`

::: devtools/client/netmonitor/src/actions/ui.js:28
(Diff revision 1)
>   * Change network details panel.
>   *
>   * @param {boolean} open - expected network details panel open state
>   */
>  function openNetworkDetails(open) {
> -  return {
> +  return (dispatch, getState) =>{

nit: there should be space between `=>` and `{`

::: devtools/client/netmonitor/src/actions/ui.js:34
(Diff revision 1)
> +    const displayedRequests = getDisplayedRequests(getState());
> +
> +    return dispatch({
> -    type: OPEN_NETWORK_DETAILS,
> +      type: OPEN_NETWORK_DETAILS,
> -    open,
> +      open,
> +      displayedRequests,

I think that it's a bit better to pass just the current first request ID through the action object.

You can get the first request from the `displayedRequests` and pass it in as e.g. `defaultSelectedId` prop.

::: devtools/client/netmonitor/src/reducers/requests.js:182
(Diff revision 1)
>          return nextState;
>        }
>  
> -      if (!state.selectedId && state.requests.size > 0) {
> -        nextState.selectedId = [...state.requests.values()][0].id;
> +      if (!state.selectedId && action.displayedRequests.size > 0) {
> +        nextState.selectedId = [...action.displayedRequests.values()][0].id;
>          return nextState;

So, this code can use `defaultSelectedId` prop as described in the other comment.

Comment 10

[mayank9856]

Comment on attachment 8964195 [details]
Bug 1442884 - Show request details button does not work when the first request list item is not shown using any filters.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232940/diff/1-2/

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8964195 [details]
Bug 1442884 - Show request details button does not work when the first request list item is not shown using any filters.

https://reviewboard.mozilla.org/r/232940/#review238774

Thanks for the update!

Plase see my inline comments

Honza

::: devtools/client/netmonitor/src/actions/ui.js:31
(Diff revision 2)
>   */
>  function openNetworkDetails(open) {
> -  return {
> +  return (dispatch, getState) => {
> +    const state = getState();
> +    const visibleRequestItems = getDisplayedRequests(state);
> +    let defaultSelectedId = null;

No need to initialize to null

::: devtools/client/netmonitor/src/actions/ui.js:34
(Diff revision 2)
> +    const state = getState();
> +    const visibleRequestItems = getDisplayedRequests(state);
> +    let defaultSelectedId = null;
> +
> +    if (open && !state.selectedId && visibleRequestItems.size > 0) {
> +      defaultSelectedId = [...visibleRequestItems.values()][0].id;

`visibleRequestItems` is an array, so you can do:

let defaultSelectedId = visibleRequestItems.length ? visibleRequestItems[0] : null;

::: devtools/client/netmonitor/src/actions/ui.js:42
(Diff revision 2)
> +    return dispatch({
> -    type: OPEN_NETWORK_DETAILS,
> +      type: OPEN_NETWORK_DETAILS,
> -    open,
> +      open,
> +      defaultSelectedId,
> +    });
>    };

In any case, this method should only set the `defaultSelectedId` prop and let the rest up to the reducer.

::: devtools/client/netmonitor/src/reducers/requests.js:175
(Diff revision 2)
> -        nextState.selectedId = [...state.requests.values()][0].id;
> -        return nextState;
> -      }
> -
> -      return state;
>      }

The reducer should stay the same and use `defaultSelectedId` as needed.

::: devtools/client/netmonitor/test/browser_net_pane-network-details.js:1
(Diff revision 2)
> +/* Any copyright is dedicated to the Public Domain.

The test seems to be too complex for the issue we are fixing here. It rather looks like it's testing filters...

I think that we should only test the following:

Show request details button does not work when the first request list item is not shown using any filters.

Could we just follow STRs from comment #0 and significantly simplify the test?

Comment 12

[mayank9856]

Comment on attachment 8964195 [details]
Bug 1442884 - Show request details button does not work when the first request list item is not shown using any filters.

https://reviewboard.mozilla.org/r/232940/#review238774

> The test seems to be too complex for the issue we are fixing here. It rather looks like it's testing filters...
> 
> I think that we should only test the following:
> 
> Show request details button does not work when the first request list item is not shown using any filters.
> 
> Could we just follow STRs from comment #0 and significantly simplify the test?

So, I had three intentions for the tests.
1. The panel should work when all requests are shown
2. The panel should work when either one or more than once (combination) of filters are applied
3. The panel should work when the filter is applied via the filter box - open when filter matches a request and also when it doesn't


This is how I've structured the test as well.
The reason I kept the tests for filters is to make sure that the requests currently visible is not always the request at the index 0 and indeed we test with a few requests when the first request is hidden, which is what the first comment in the bug mentions.

I'll go ahead and remove the tests for filters and only include tests for the four scenarios mentioned above.

Comment 13

[mayank9856]

Comment on attachment 8964195 [details]
Bug 1442884 - Show request details button does not work when the first request list item is not shown using any filters.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232940/diff/2-3/

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8964195 [details]
Bug 1442884 - Show request details button does not work when the first request list item is not shown using any filters.

https://reviewboard.mozilla.org/r/232940/#review239094

The test is a lot better now!

Please see my inline comments.

Also, check all the text messages, some of them seem to be incorrect.

This is almost ready.

Thanks,
Honza

::: devtools/client/netmonitor/src/actions/ui.js:30
(Diff revision 3)
>   * @param {boolean} open - expected network details panel open state
>   */
>  function openNetworkDetails(open) {
> -  return {
> +  return (dispatch, getState) => {
> +    const visibleRequestItems = getDisplayedRequests(getState());
> +    let defaultSelectedId = visibleRequestItems.size ? visibleRequestItems[0].id : null;

Please use `length` not `size` (just like the example in my previous comment)

The reason is that `size` is a helper implemented here:

https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/devtools/client/netmonitor/src/selectors/requests.js#87

The helper has been introduced [1] when removing ImmutableJS (to simplify the process) and should  be removed at some point.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1408182

::: devtools/client/netmonitor/test/browser_net_pane-network-details.js:8
(Diff revision 3)
> +
> +"use strict";
> +
> +/**
> + * Test the action of request details panel when filters are applied.
> + */

It would be great to be more descriptive in the comment. Explaining that the test is verifying that it should be possible to open the network details panel if there are requests in the list, etc.

::: devtools/client/netmonitor/test/browser_net_pane-network-details.js:30
(Diff revision 3)
> +]);
> +
> +const REQUESTS_WITH_MEDIA_AND_FLASH_AND_WS = REQUESTS_WITH_MEDIA_AND_FLASH.concat([
> +  /* "Upgrade" is a reserved header and can not be set on XMLHttpRequest */
> +  { url: "sjs_content-type-test-server.sjs?fmt=ws" },
> +]);

Since only the REQUESTS_WITH_MEDIA_AND_FLASH_AND_WS array is actually used in the test, can you define just one big `REQUESTS_WITH_MEDIA_AND_FLASH_AND_WS` array?

::: devtools/client/netmonitor/test/browser_net_pane-network-details.js:93
(Diff revision 3)
> +      // Toggle switch should be enabled only when there are visible requests
> +      is(toggleButton.hasAttribute("disabled"), false,
> +          "The pane toggle button should be enabled.");
> +    } else {
> +      is(toggleButton.hasAttribute("disabled"), true,
> +          "The pane toggle button should be enabled.");

The text should say "...should be disabled.", right?

::: devtools/client/netmonitor/test/browser_net_pane-network-details.js:124
(Diff revision 3)
> +
> +      // Close the panel
> +      EventUtils.sendMouseEvent({ type: "click" }, toggleButton);
> +    } else {
> +      is(toggleButton.hasAttribute("disabled"), true,
> +          "The pane toggle button should be enabled.");

Is the text message correct? enabled/disabled

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

A tip:

When running a test locally I am using 'verify' and 'headless' arguments for 'mach test'.

verify - runs the test many times in different modes
headless - runs with no UI, so you can continue using the machine without messing the focus, etc.

Example:
mach test devtools/client/netmonitor/test/browser_net_pane-network-details.js --verify --headless

Honza

Comment 16

[mayank9856]

Attachment: Bug 1442884 - Show request details button does not work when the first request list item is not shown using any filters.

Review commit: https://reviewboard.mozilla.org/r/233732/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233732/

Comment 17

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8964987 [details]
Bug 1442884 - Show request details button does not work when the first request list item is not shown using any filters.

https://reviewboard.mozilla.org/r/233732/#review240070

Great work her!

R+, but please resove my little nit comments

Thanks,
Honza

::: devtools/client/netmonitor/test/browser_net_pane-network-details.js:8
(Diff revision 1)
>  
>  "use strict";
>  
>  /**
>   * Test the action of request details panel when filters are applied.
> + * If there is any visible requests, the first request from the

nit: if there are any ...

::: devtools/client/netmonitor/test/browser_net_pane-network-details.js:10
(Diff revision 1)
>  
>  /**
>   * Test the action of request details panel when filters are applied.
> + * If there is any visible requests, the first request from the
> + *   list of visible requests should be displayed in the network
> + *   details panel

nit: There should be just one space between the star and the comment:

```
* list of visible requests should be displayed in the network
* details panel
```

Comment 18

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Also, don't forget to mark the previous patch as obsolete.

Honza

Comment 19

[mayank9856]

Attachment: Bug 1442884 - Show request details button does not work when the first request list item is not shown using any filters.

Review commit: https://reviewboard.mozilla.org/r/234512/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234512/

Comment 20

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8965661 [details]
Bug 1442884 - Show request details button does not work when the first request list item is not shown using any filters.

https://reviewboard.mozilla.org/r/234512/#review240128

Comment 21

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Please merge all patches into one.

Thanks,
Honza

Comment 22

[mayank9856]

Comment on attachment 8964195 [details]
Bug 1442884 - Show request details button does not work when the first request list item is not shown using any filters.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232940/diff/3-4/

Comment 23

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Push to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5090659ce8909443844a88109cb74a5cd46e719
Honza

Comment 24

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8964195 [details]
Bug 1442884 - Show request details button does not work when the first request list item is not shown using any filters.

https://reviewboard.mozilla.org/r/232940/#review240484

Try looks good, ready to land.

Thanks,
Honza

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e7359cedcc1
Show request details button does not work when the first request list item is not shown using any filters. r=Honza

Comment 26

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/2e7359cedcc1

Comment 27

[Liz Henry (:lizzard)]

Too late to fix in 59. Honza, what do you think about uplift to 60? We could let this ride with 61 but if you think it's safe to uplift please go ahead and request it.

Comment 28

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8964195 [details]
Bug 1442884 - Show request details button does not work when the first request list item is not shown using any filters.

Approval Request Comment
[Feature/Bug causing the regression]: n/a
[User impact if declined]: HTTP request details side bar can't be opened in some cases
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low risk
[Why is the change risky/not risky?]: only for developers, covered by tests
[String changes made/needed]: n/a

Comment 29

[Julien Cristau [:jcristau]]

Comment on attachment 8964195 [details]
Bug 1442884 - Show request details button does not work when the first request list item is not shown using any filters.

netmonitor fix, approved for 60.0b12

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/4f4f03584f6a

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/08762d09e874

Comment 32

[Andrei Vaida [:avaida]]

(In reply to Jan Honza Odvarko [:Honza] from comment #28)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]:  no

Setting qe-verify- based on Jan's assessment on manual testing needs and the fact that this fix has automated tests.