Navigation: Move SectionNav to AppChrome (#64391)

* move section nav to app chrome

* unit tests

* Move SectionNav to AppChrome folder

* fix duplicate variable rendering

---------

Co-authored-by: Torkel Ödegaard <torkel@grafana.com>

.betterer.results

@@ -1548,6 +1548,9 @@ exports[`better eslint`] = {
       [0, 0, 0, "Unexpected any. Specify a different type.", "5"],
       [0, 0, 0, "Unexpected any. Specify a different type.", "6"]
     ],
+    "public/app/core/components/AppChrome/SectionNav/SectionNavItem.tsx:5381": [
+      [0, 0, 0, "Use data-testid for E2E selectors instead of aria-label", "0"]
+    ],
     "public/app/core/components/DynamicImports/SafeDynamicImport.tsx:5381": [
       [0, 0, 0, "Unexpected any. Specify a different type.", "0"]
     ],
@@ -1650,9 +1653,6 @@ exports[`better eslint`] = {
       [0, 0, 0, "Do not use any type assertions.", "85"],
       [0, 0, 0, "Unexpected any. Specify a different type.", "86"]
     ],
-    "public/app/core/components/PageNew/SectionNavItem.tsx:5381": [
-      [0, 0, 0, "Use data-testid for E2E selectors instead of aria-label", "0"]
-    ],
     "public/app/core/components/PanelTypeFilter/PanelTypeFilter.tsx:5381": [
       [0, 0, 0, "Unexpected any. Specify a different type.", "0"],
       [0, 0, 0, "Unexpected any. Specify a different type.", "1"]

public/app/core/components/AppChrome/AppChrome.test.tsx

@@ -0,0 +1,108 @@
+import { render, screen } from '@testing-library/react';
+import { KBarProvider } from 'kbar';
+import React, { ReactNode } from 'react';
+import { TestProvider } from 'test/helpers/TestProvider';
+import { getGrafanaContextMock } from 'test/mocks/getGrafanaContextMock';
+
+import { ArrayVector, DataFrame, DataFrameView, FieldType, NavModelItem } from '@grafana/data';
+import { config } from '@grafana/runtime';
+import { HOME_NAV_ID } from 'app/core/reducers/navModel';
+import { DashboardQueryResult, getGrafanaSearcher, QueryResponse } from 'app/features/search/service';
+
+import { Page } from '../PageNew/Page';
+
+import { AppChrome } from './AppChrome';
+
+const pageNav: NavModelItem = {
+  text: 'pageNav title',
+  children: [
+    { text: 'pageNav child1', url: '1', active: true },
+    { text: 'pageNav child2', url: '2' },
+  ],
+};
+
+const searchData: DataFrame = {
+  fields: [
+    { name: 'kind', type: FieldType.string, config: {}, values: new ArrayVector([]) },
+    { name: 'name', type: FieldType.string, config: {}, values: new ArrayVector([]) },
+    { name: 'uid', type: FieldType.string, config: {}, values: new ArrayVector([]) },
+    { name: 'url', type: FieldType.string, config: {}, values: new ArrayVector([]) },
+    { name: 'tags', type: FieldType.other, config: {}, values: new ArrayVector([]) },
+    { name: 'location', type: FieldType.string, config: {}, values: new ArrayVector([]) },
+  ],
+  length: 0,
+};
+
+const mockSearchResult: QueryResponse = {
+  isItemLoaded: jest.fn(),
+  loadMoreItems: jest.fn(),
+  totalRows: searchData.length,
+  view: new DataFrameView<DashboardQueryResult>(searchData),
+};
+
+const setup = (children: ReactNode) => {
+  config.bootData.navTree = [
+    {
+      id: HOME_NAV_ID,
+      text: 'Home',
+    },
+    {
+      text: 'Section name',
+      id: 'section',
+      url: 'section',
+      children: [
+        { text: 'Child1', id: 'child1', url: 'section/child1' },
+        { text: 'Child2', id: 'child2', url: 'section/child2' },
+      ],
+    },
+  ];
+
+  const context = getGrafanaContextMock();
+
+  const renderResult = render(
+    <KBarProvider>
+      <TestProvider grafanaContext={context}>
+        <AppChrome>
+          <div data-testid="page-children">{children}</div>
+        </AppChrome>
+      </TestProvider>
+    </KBarProvider>
+  );
+
+  return { renderResult, context };
+};
+
+describe('AppChrome', () => {
+  beforeAll(() => {
+    // need to mock out the search service since kbar calls it to fetch recent dashboards
+    jest.spyOn(getGrafanaSearcher(), 'search').mockResolvedValue(mockSearchResult);
+  });
+
+  afterEach(() => {
+    jest.clearAllMocks();
+  });
+
+  it('should render section nav model based on navId', async () => {
+    setup(<Page navId="child1">Children</Page>);
+    expect(await screen.findByTestId('page-children')).toBeInTheDocument();
+
+    expect(screen.getByRole('tab', { name: 'Tab Section name' })).toBeInTheDocument();
+    expect(screen.getByRole('tab', { name: 'Tab Child1' })).toBeInTheDocument();
+    expect(screen.getByRole('tab', { name: 'Tab Child1' })).toBeInTheDocument();
+    expect(screen.getAllByRole('tab').length).toBe(3);
+  });
+
+  it('should render section nav model based on navId and item page nav', async () => {
+    setup(
+      <Page navId="child1" pageNav={pageNav}>
+        Children
+      </Page>
+    );
+    expect(await screen.findByTestId('page-children')).toBeInTheDocument();
+
+    expect(screen.getByRole('tab', { name: 'Tab Section name' })).toBeInTheDocument();
+    expect(screen.getByRole('heading', { name: 'pageNav title' })).toBeInTheDocument();
+    expect(screen.getByRole('tab', { name: 'Tab Child1' })).toBeInTheDocument();
+    expect(screen.getByRole('tab', { name: 'Tab pageNav child1' })).toBeInTheDocument();
+  });
+});

public/app/core/components/AppChrome/AppChrome.tsx

@@ -1,7 +1,7 @@
 import { css, cx } from '@emotion/css';
 import React, { PropsWithChildren } from 'react';
 
-import { GrafanaTheme2 } from '@grafana/data';
+import { GrafanaTheme2, PageLayoutType } from '@grafana/data';
 import { useStyles2 } from '@grafana/ui';
 import { useGrafana } from 'app/core/context/GrafanaContext';
 import { CommandPalette } from 'app/features/commandPalette/CommandPalette';
@@ -9,6 +9,7 @@ import { KioskMode } from 'app/types';
 
 import { MegaMenu } from './MegaMenu/MegaMenu';
 import { NavToolbar } from './NavToolbar/NavToolbar';
+import { SectionNav } from './SectionNav/SectionNav';
 import { TopSearchBar } from './TopBar/TopSearchBar';
 import { TOP_BAR_LEVEL_HEIGHT } from './types';
 
@@ -38,7 +39,7 @@ export function AppChrome({ children }: Props) {
           {!searchBarHidden && <TopSearchBar />}
           <NavToolbar
             searchBarHidden={searchBarHidden}
-            sectionNav={state.sectionNav}
+            sectionNav={state.sectionNav.node}
             pageNav={state.pageNav}
             actions={state.actions}
             onToggleSearchBar={chrome.onToggleSearchBar}
@@ -47,7 +48,12 @@ export function AppChrome({ children }: Props) {
           />
         </div>
       )}
-      <div className={contentClass}>{children}</div>
+      <div className={contentClass}>
+        <div className={styles.panes}>
+          {state.layout === PageLayoutType.Standard && state.sectionNav && <SectionNav model={state.sectionNav} />}
+          <div className={styles.pageContainer}>{children}</div>
+        </div>
+      </div>
       {!state.chromeless && (
         <>
           <MegaMenu searchBarHidden={searchBarHidden} onClose={() => chrome.setMegaMenu(false)} />
@@ -88,5 +94,21 @@ const getStyles = (theme: GrafanaTheme2) => {
       flexDirection: 'column',
       borderBottom: `1px solid ${theme.colors.border.weak}`,
     }),
+    panes: css({
+      label: 'page-panes',
+      display: 'flex',
+      height: '100%',
+      width: '100%',
+      flexGrow: 1,
+      minHeight: 0,
+      flexDirection: 'column',
+      [theme.breakpoints.up('md')]: {
+        flexDirection: 'row',
+      },
+    }),
+    pageContainer: css({
+      label: 'page-container',
+      flexGrow: 1,
+    }),
   };
 };

public/app/core/components/AppChrome/AppChromeService.test.tsx

@@ -13,25 +13,34 @@ describe('AppChromeService', () => {
     let stateChanges = 0;
 
     chromeService.state.subscribe(() => stateChanges++);
-    chromeService.update({ sectionNav: { text: 'hello' }, pageNav: { text: 'test', url: 'A' } });
-    chromeService.update({ sectionNav: { text: 'hello' }, pageNav: { text: 'test', url: 'A' } });
+    chromeService.update({
+      sectionNav: { node: { text: 'hello' }, main: { text: '' } },
+      pageNav: { text: 'test', url: 'A' },
+    });
+    chromeService.update({
+      sectionNav: { node: { text: 'hello' }, main: { text: '' } },
+      pageNav: { text: 'test', url: 'A' },
+    });
 
     expect(stateChanges).toBe(2);
 
     // if url change we should update
-    chromeService.update({ sectionNav: { text: 'hello' }, pageNav: { text: 'test', url: 'new/url' } });
+    chromeService.update({
+      sectionNav: { node: { text: 'hello' }, main: { text: '' } },
+      pageNav: { text: 'test', url: 'new/url' },
+    });
     expect(stateChanges).toBe(3);
 
     // if active child changed should update state
     chromeService.update({
-      sectionNav: { text: 'hello' },
+      sectionNav: { node: { text: 'hello' }, main: { text: '' } },
       pageNav: { text: 'test', url: 'A', children: [{ text: 'child', active: true }] },
     });
     expect(stateChanges).toBe(4);
 
     // If active child is the same we should not update state
     chromeService.update({
-      sectionNav: { text: 'hello' },
+      sectionNav: { node: { text: 'hello' }, main: { text: '' } },
       pageNav: { text: 'test', url: 'A', children: [{ text: 'child', active: true }] },
     });
     expect(stateChanges).toBe(4);

public/app/core/components/AppChrome/AppChromeService.tsx

@@ -1,7 +1,7 @@
 import { useObservable } from 'react-use';
 import { BehaviorSubject } from 'rxjs';
 
-import { AppEvents, NavModelItem, UrlQueryValue } from '@grafana/data';
+import { AppEvents, NavModel, NavModelItem, PageLayoutType, UrlQueryValue } from '@grafana/data';
 import { locationService, reportInteraction } from '@grafana/runtime';
 import appEvents from 'app/core/app_events';
 import { t } from 'app/core/internationalization';
@@ -13,12 +13,13 @@ import { RouteDescriptor } from '../../navigation/types';
 
 export interface AppChromeState {
   chromeless?: boolean;
-  sectionNav: NavModelItem;
+  sectionNav: NavModel;
   pageNav?: NavModelItem;
   actions?: React.ReactNode;
   searchBarHidden?: boolean;
   megaMenuOpen?: boolean;
   kioskMode: KioskMode | null;
+  layout: PageLayoutType;
 }
 
 export class AppChromeService {
@@ -28,9 +29,10 @@ export class AppChromeService {
 
   readonly state = new BehaviorSubject<AppChromeState>({
     chromeless: true, // start out hidden to not flash it on pages without chrome
-    sectionNav: { text: t('nav.home.title', 'Home') },
+    sectionNav: { node: { text: t('nav.home.title', 'Home') }, main: { text: '' } },
     searchBarHidden: store.getBool(this.searchBarStorageKey, false),
     kioskMode: null,
+    layout: PageLayoutType.Standard,
   });
 
   setMatchedRoute(route: RouteDescriptor) {
@@ -50,8 +52,9 @@ export class AppChromeService {
     if (!this.routeChangeHandled) {
       newState.actions = undefined;
       newState.pageNav = undefined;
-      newState.sectionNav = { text: t('nav.home.title', 'Home') };
+      newState.sectionNav = { node: { text: t('nav.home.title', 'Home') }, main: { text: '' } };
       newState.chromeless = this.currentRoute?.chromeless;
+      newState.layout = PageLayoutType.Standard;
       this.routeChangeHandled = true;
     }
 
@@ -74,7 +77,7 @@ export class AppChromeService {
     if (newState.sectionNav !== current.sectionNav || newState.pageNav !== current.pageNav) {
       if (
         newState.actions === current.actions &&
-        navItemsAreTheSame(newState.sectionNav, current.sectionNav) &&
+        navItemsAreTheSame(newState.sectionNav.node, current.sectionNav.node) &&
         navItemsAreTheSame(newState.pageNav, current.pageNav)
       ) {
         return true;

public/app/core/components/AppChrome/AppChromeUpdate.tsx

@@ -1,4 +1,4 @@
-import React, { useEffect } from 'react';
+import React, { useLayoutEffect } from 'react';
 
 import { useGrafana } from 'app/core/context/GrafanaContext';
 
@@ -12,7 +12,9 @@ export interface AppChromeUpdateProps {
 export const AppChromeUpdate = React.memo<AppChromeUpdateProps>(({ actions }: AppChromeUpdateProps) => {
   const { chrome } = useGrafana();
 
-  useEffect(() => {
+  // We use useLayoutEffect here to make sure that the chrome is updated before the page is rendered
+  // This prevents flickering actions when going from one dashbaord to another for example
+  useLayoutEffect(() => {
     chrome.update({ actions });
   });
   return null;

public/app/core/components/Page/Page.tsx

@@ -44,7 +44,7 @@ export const OldPage: PageType = ({
     if (navModel) {
       // This is needed for chrome to update it's chromeless state
       chrome.update({
-        sectionNav: navModel.node,
+        sectionNav: navModel,
       });
     } else {
       // Need to trigger a chrome state update for the route change to be processed

public/app/core/components/PageNew/Page.test.tsx

@@ -3,7 +3,7 @@ import React from 'react';
 import { TestProvider } from 'test/helpers/TestProvider';
 import { getGrafanaContextMock } from 'test/mocks/getGrafanaContextMock';
 
-import { NavModelItem } from '@grafana/data';
+import { NavModelItem, PageLayoutType } from '@grafana/data';
 import { config } from '@grafana/runtime';
 import { HOME_NAV_ID } from 'app/core/reducers/navModel';
 
@@ -65,28 +65,11 @@ describe('Render', () => {
     expect(screen.getAllByRole('tab').length).toBe(2);
   });
 
-  it('should render section nav model based on navId', async () => {
-    setup({ navId: 'child1' });
-
-    expect(screen.getByRole('tab', { name: 'Tab Section name' })).toBeInTheDocument();
-    expect(screen.getByRole('tab', { name: 'Tab Child1' })).toBeInTheDocument();
-    expect(screen.getByRole('tab', { name: 'Tab Child1' })).toBeInTheDocument();
-    expect(screen.getAllByRole('tab').length).toBe(3);
-  });
-
-  it('should update chrome with section and pageNav', async () => {
-    const { context } = setup({ navId: 'child1', pageNav });
-    expect(context.chrome.state.getValue().sectionNav.id).toBe('child1');
+  it('should update chrome with section, pageNav and layout', async () => {
+    const { context } = setup({ navId: 'child1', pageNav, layout: PageLayoutType.Canvas });
+    expect(context.chrome.state.getValue().sectionNav.node.id).toBe('child1');
     expect(context.chrome.state.getValue().pageNav).toBe(pageNav);
-  });
-
-  it('should render section nav model based on navId and item page nav', async () => {
-    setup({ navId: 'child1', pageNav });
-
-    expect(screen.getByRole('tab', { name: 'Tab Section name' })).toBeInTheDocument();
-    expect(screen.getByRole('heading', { name: 'pageNav title' })).toBeInTheDocument();
-    expect(screen.getByRole('tab', { name: 'Tab Child1' })).toBeInTheDocument();
-    expect(screen.getByRole('tab', { name: 'Tab pageNav child1' })).toBeInTheDocument();
+    expect(context.chrome.state.getValue().layout).toBe(PageLayoutType.Canvas);
   });
 
   it('should update document title', async () => {

public/app/core/components/PageNew/Page.tsx

@@ -1,6 +1,6 @@
 // Libraries
 import { css, cx } from '@emotion/css';
-import React, { useEffect } from 'react';
+import React, { useLayoutEffect } from 'react';
 
 import { GrafanaTheme2, PageLayoutType } from '@grafana/data';
 import { CustomScrollbar, useStyles2 } from '@grafana/ui';
@@ -13,7 +13,6 @@ import { usePageTitle } from '../Page/usePageTitle';
 import { PageContents } from './PageContents';
 import { PageHeader } from './PageHeader';
 import { PageTabs } from './PageTabs';
-import { SectionNav } from './SectionNav';
 
 export const Page: PageType = ({
   navId,
@@ -39,38 +38,36 @@ export const Page: PageType = ({
 
   const pageHeaderNav = pageNav ?? navModel?.node;
 
-  useEffect(() => {
+  // We use useLayoutEffect here to make sure that the chrome is updated before the page is rendered
+  // This prevents flickering sectionNav when going from dashbaord to settings for example
+  useLayoutEffect(() => {
     if (navModel) {
       chrome.update({
-        sectionNav: navModel.node,
+        sectionNav: navModel,
         pageNav: pageNav,
+        layout: layout,
       });
     }
-  }, [navModel, pageNav, chrome]);
+  }, [navModel, pageNav, chrome, layout]);
 
   return (
     <div className={cx(styles.wrapper, className)} {...otherProps}>
       {layout === PageLayoutType.Standard && (
-        <div className={styles.panes}>
-          {navModel && <SectionNav model={navModel} />}
-          <div className={styles.pageContainer}>
-            <CustomScrollbar autoHeightMin={'100%'} scrollTop={scrollTop} scrollRefCallback={scrollRef}>
-              <div className={styles.pageInner}>
-                {pageHeaderNav && (
-                  <PageHeader
-                    actions={actions}
-                    navItem={pageHeaderNav}
-                    renderTitle={renderTitle}
-                    info={info}
-                    subTitle={subTitle}
-                  />
-                )}
-                {pageNav && pageNav.children && <PageTabs navItem={pageNav} />}
-                <div className={styles.pageContent}>{children}</div>
-              </div>
-            </CustomScrollbar>
+        <CustomScrollbar autoHeightMin={'100%'} scrollTop={scrollTop} scrollRefCallback={scrollRef}>
+          <div className={styles.pageInner}>
+            {pageHeaderNav && (
+              <PageHeader
+                actions={actions}
+                navItem={pageHeaderNav}
+                renderTitle={renderTitle}
+                info={info}
+                subTitle={subTitle}
+              />
+            )}
+            {pageNav && pageNav.children && <PageTabs navItem={pageNav} />}
+            <div className={styles.pageContent}>{children}</div>
           </div>
-        </div>
+        </CustomScrollbar>
       )}
       {layout === PageLayoutType.Canvas && (
         <CustomScrollbar autoHeightMin={'100%'} scrollTop={scrollTop} scrollRefCallback={scrollRef}>
@@ -102,22 +99,6 @@ const getStyles = (theme: GrafanaTheme2) => {
       flexDirection: 'column',
       minHeight: 0,
     }),
-    panes: css({
-      label: 'page-panes',
-      display: 'flex',
-      height: '100%',
-      width: '100%',
-      flexGrow: 1,
-      minHeight: 0,
-      flexDirection: 'column',
-      [theme.breakpoints.up('md')]: {
-        flexDirection: 'row',
-      },
-    }),
-    pageContainer: css({
-      label: 'page-container',
-      flexGrow: 1,
-    }),
     pageContent: css({
       label: 'page-content',
       flexGrow: 1,

public/app/features/dashboard/components/DashboardSettings/DashboardSettings.test.tsx

@@ -1,38 +0,0 @@
-import { render, screen } from '@testing-library/react';
-import React from 'react';
-import { TestProvider } from 'test/helpers/TestProvider';
-
-import { NavModel, NavModelItem } from '@grafana/data';
-import { BackendSrv, setBackendSrv } from '@grafana/runtime';
-
-import { createDashboardModelFixture } from '../../state/__fixtures__/dashboardFixtures';
-
-import { DashboardSettings } from './DashboardSettings';
-
-setBackendSrv({
-  get: jest.fn().mockResolvedValue([]),
-} as unknown as BackendSrv);
-
-describe('DashboardSettings', () => {
-  it('pressing escape navigates away correctly', async () => {
-    const dashboard = createDashboardModelFixture(
-      {
-        title: 'Foo',
-      },
-      {
-        folderId: 1,
-      }
-    );
-
-    const sectionNav: NavModel = { main: { text: 'Dashboards' }, node: { text: 'Dashboards' } };
-    const pageNav: NavModelItem = { text: 'My cool dashboard' };
-
-    render(
-      <TestProvider>
-        <DashboardSettings editview="settings" dashboard={dashboard} sectionNav={sectionNav} pageNav={pageNav} />
-      </TestProvider>
-    );
-
-    expect(await screen.findByRole('tab', { name: 'Tab Settings' })).toBeInTheDocument();
-  });
-});

public/app/features/explore/ExplorePage.tsx

@@ -49,7 +49,7 @@ export function ExplorePage(props: GrafanaRouteComponentProps<{}, ExploreQueryPa
   useEffect(() => {
     //This is needed for breadcrumbs and topnav.
     //We should probably abstract this out at some point
-    chrome.update({ sectionNav: navModel.node });
+    chrome.update({ sectionNav: navModel });
   }, [chrome, navModel]);
 
   useEffect(() => {