Nav Unification - clicking on sections causes UI to "jump" and triggers other sections to close.

[getdave]

Open a menu section. Then click on another menu section. This will sometimes cause the other menu sections to toggle closed.

For example in the screencapture below:

• Pages is toggled open.
• Tools is clicked.
• Tools opens and Pages closes. This is not expected.

I believe this is due to the sidebar completely re-rendering when the route path prop changes. Given that path is passed down the React tree several levels deep any change to this will cause the entire nav to render (including the Site Switcher and Notices).

Here's a flamegraph from the React profiler showing the render profile when the Pages menu item is clicked.

[getdave]

As I may not be able to see this over the line (see p242cR-keI-p2) here's a proposed route forward for whoever picks this up.

Original Plan

The key problems are:

1. Optimising the messagePath and jitms props going to SiteNotice and JITM components.
2. Optimise and improve the state management of JITMs in Redux to avoid the JITMs in state being "reset" on navigation to a new section within the UI.
3. Merge the above without breaking existing implementations.

We have x3 PRs all try to address the messagePath prop on the component side. I list them below in the order of priority based on my preferred implementation:

1. Fix SiteNotice re-renders due to changing messagePath #46713 (preferred route) <-- look at this one!
2. Stop section changes from re-rendering SiteNotice #46688 (outdated)
3. Allow lazy build of messagePath prop to reduce SiteNotice re-renders #46676 (outdated)

I would personally go with #1 as it seems like the best overall approach. Given the potential for this to affect JITMs showing across Calypso I would recommend we get input from at least 1 external reviewer. Ideally @sixhours if possible as she has worked on this previously.

Following on from that we need to merge whichever approach we take for messagePath and then rebase #46677 which handles optimisation of the JITMs in state.

Note: as #46677 was based on #46676 we'll need to remove those changes first before rebasing.

Once that is done, then we need some final external reviews on #46677 in order to merge and deploy. With that done the "jumping" behaviour should be removed (from both unified nav and the current Calypso nav!).

Alternative Plan

Another solution was offered by @jsnajdr which is to ignore the section portion of the messagePath as it's not in use. See p1603458714018300-slack-calypso-framework. That would allow us to stop passing a different sectionName for each route and simply hard code in calypso:site:sidebar_notice.

He also suggested that we could simplify the whole thing and only include the fixes to the JITMs cache in state and that would solve the problem of rendering. I've tried this by cherry picking a subset of the changes and it does appear that it does resolve the issue without requiring the fixes to messagePath.

cc @obenland @davemart-in

[mreishus]

Update:

• Fix SiteNotice re-renders due to changing messagePath #46713 has been approved (including external); I will merge it on Monday (2020-10-26)
• Stop section changes from re-rendering SiteNotice #46688 and Allow lazy build of messagePath prop to reduce SiteNotice re-renders #46676 are superseded by Fix SiteNotice re-renders due to changing messagePath #46713
• Cache latest JITMs in state to reduce SiteNotice and JITM component rerenders #46677 is still in review and attacks the problem from a different angle; still in review, but is looking good.

Either one of (#46713, #46677) is sufficient to fix the jumpiness, but by merging both we'll fix it and get a small performance boost, too.