Nav: Fix alerting links/special cases not selecting the right MegaMenu item (#85336)

* Render current MegaMenu link as `aria-current=page`

* Add overrides capability for mega menu links

* Pass pageNav into getActiveItem so we can use override capability

* Test MegaMenu special cases for starred & dashboards

* Test that overrides for megamenu util works correctly

* Alpha-sort megamenu overrides

* Refactor util for getting active item for megamenu

Update parameters to getActiveItem

Update tests for getActiveItem

* Fix test for starred dashboard and remove query param test

Query param case happens differently in real app and is fiddly to test here

* handle edge cases

* restore handling home page test

* fix dashboard settings

* handle starring properly

---------

Co-authored-by: Ashley Harrison <ashley.harrison@grafana.com>

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

@@ -2,14 +2,13 @@ import { render, screen } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
 import React from 'react';
 import { Router } from 'react-router-dom';
+import { TestProvider } from 'test/helpers/TestProvider';
 import { getGrafanaContextMock } from 'test/mocks/getGrafanaContextMock';
 
 import { NavModelItem } from '@grafana/data';
 import { selectors } from '@grafana/e2e-selectors';
 import { locationService } from '@grafana/runtime';
 
-import { TestProvider } from '../../../../../test/helpers/TestProvider';
-
 import { MegaMenu } from './MegaMenu';
 
 const setup = () => {

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

@@ -32,7 +32,7 @@ export const MegaMenu = React.memo(
       .filter((item) => item.id !== 'profile' && item.id !== 'help')
       .map((item) => enrichWithInteractionTracking(item, state.megaMenuDocked));
 
-    const activeItem = getActiveItem(navItems, location.pathname);
+    const activeItem = getActiveItem(navItems, state.sectionNav.node, location.pathname);
 
     const handleDockedMenu = () => {
       chrome.setMegaMenuDocked(!state.megaMenuDocked);

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

@@ -38,6 +38,7 @@ export function MegaMenuItemText({ children, isActive, onClick, target, url }: P
       href={url}
       target={target}
       onClick={onClick}
+      {...(isActive && { 'aria-current': 'page' })}
     >
       {linkContent}
     </LinkComponent>

public/app/core/components/AppChrome/MegaMenu/utils.test.ts

@@ -1,7 +1,7 @@
-import { GrafanaConfig, locationUtil, NavModelItem } from '@grafana/data';
+import { NavModelItem } from '@grafana/data';
 import { ContextSrv, setContextSrv } from 'app/core/services/context_srv';
 
-import { enrichHelpItem, getActiveItem, isMatchOrChildMatch } from './utils';
+import { enrichHelpItem, getActiveItem } from './utils';
 
 jest.mock('../../../app_events', () => ({
   publish: jest.fn(),
@@ -44,143 +44,83 @@ describe('enrichConfigItems', () => {
   });
 });
 
-describe('isMatchOrChildMatch', () => {
-  const mockChild: NavModelItem = {
-    text: 'Child',
-    url: '/dashboards/child',
-  };
-  const mockItemToCheck: NavModelItem = {
-    text: 'Dashboards',
-    url: '/dashboards',
-    children: [mockChild],
-  };
-
-  it('returns true if the itemToCheck is an exact match with the searchItem', () => {
-    const searchItem = mockItemToCheck;
-    expect(isMatchOrChildMatch(mockItemToCheck, searchItem)).toBe(true);
-  });
-
-  it('returns true if the itemToCheck has a child that matches the searchItem', () => {
-    const searchItem = mockChild;
-    expect(isMatchOrChildMatch(mockItemToCheck, searchItem)).toBe(true);
-  });
-
-  it('returns false otherwise', () => {
-    const searchItem: NavModelItem = {
-      text: 'No match',
-      url: '/noMatch',
-    };
-    expect(isMatchOrChildMatch(mockItemToCheck, searchItem)).toBe(false);
-  });
-});
-
 describe('getActiveItem', () => {
+  const starredDashboardUid = 'foo';
   const mockNavTree: NavModelItem[] = [
     {
       text: 'Item',
       url: '/item',
-    },
-    {
-      text: 'Item with query param',
-      url: '/itemWithQueryParam?foo=bar',
-    },
-    {
-      text: 'Item after subpath',
-      url: '/subUrl/itemAfterSubpath',
+      id: 'item',
     },
     {
       text: 'Item with children',
       url: '/itemWithChildren',
+      id: 'item-with-children',
       children: [
         {
           text: 'Child',
           url: '/child',
+          id: 'child',
         },
       ],
     },
-    {
-      text: 'Alerting item',
-      url: '/alerting/list',
-    },
     {
       text: 'Base',
       url: '/',
+      id: 'home',
     },
     {
       text: 'Starred',
       url: '/dashboards?starred',
       id: 'starred',
+      children: [
+        {
+          id: `starred/${starredDashboardUid}`,
+          text: 'Lazy Loading',
+          url: `/d/${starredDashboardUid}/some-name`,
+        },
+      ],
     },
     {
       text: 'Dashboards',
       url: '/dashboards',
-    },
-    {
-      text: 'More specific dashboard',
-      url: '/d/moreSpecificDashboard',
+      id: 'dashboards',
     },
   ];
-  beforeEach(() => {
-    locationUtil.initialize({
-      config: { appSubUrl: '/subUrl' } as GrafanaConfig,
-      getVariablesUrlParams: () => ({}),
-      getTimeRangeForUrl: () => ({ from: 'now-7d', to: 'now' }),
-    });
-  });
 
   it('returns an exact match at the top level', () => {
-    const mockPathName = '/item';
-    expect(getActiveItem(mockNavTree, mockPathName)).toEqual({
-      text: 'Item',
-      url: '/item',
-    });
+    const mockPage: NavModelItem = {
+      text: 'Some current page',
+      id: 'item',
+    };
+    expect(getActiveItem(mockNavTree, mockPage)?.id).toEqual('item');
   });
 
-  it('returns an exact match ignoring root subpath', () => {
-    const mockPathName = '/itemAfterSubpath';
-    expect(getActiveItem(mockNavTree, mockPathName)).toEqual({
-      text: 'Item after subpath',
-      url: '/subUrl/itemAfterSubpath',
-    });
-  });
-
-  it('returns an exact match ignoring query params', () => {
-    const mockPathName = '/itemWithQueryParam?bar=baz';
-    expect(getActiveItem(mockNavTree, mockPathName)).toEqual({
-      text: 'Item with query param',
-      url: '/itemWithQueryParam?foo=bar',
-    });
+  it('returns parent item if no other matches in nav tree', () => {
+    const mockPage: NavModelItem = {
+      text: 'Some child page',
+      id: 'something-that-doesnt-exist',
+      parentItem: {
+        text: 'Some home page',
+        id: 'home',
+      },
+    };
+    expect(getActiveItem(mockNavTree, mockPage)?.id).toEqual('home');
   });
 
   it('returns an exact child match', () => {
-    const mockPathName = '/child';
-    expect(getActiveItem(mockNavTree, mockPathName)).toEqual({
-      text: 'Child',
-      url: '/child',
-    });
+    const mockPage: NavModelItem = {
+      text: 'Some child page',
+      id: 'child',
+    };
+    expect(getActiveItem(mockNavTree, mockPage)?.id).toEqual('child');
   });
 
-  it('returns the alerting link if the pathname is an alert notification', () => {
-    const mockPathName = '/alerting/notification/foo';
-    expect(getActiveItem(mockNavTree, mockPathName)).toEqual({
-      text: 'Alerting item',
-      url: '/alerting/list',
-    });
-  });
-
-  it('returns the dashboards route link if the pathname starts with /d/', () => {
-    const mockPathName = '/d/foo';
-    expect(getActiveItem(mockNavTree, mockPathName)).toEqual({
-      text: 'Dashboards',
-      url: '/dashboards',
-    });
-  });
-
-  it('returns a more specific link if one exists', () => {
-    const mockPathName = '/d/moreSpecificDashboard';
-    expect(getActiveItem(mockNavTree, mockPathName)).toEqual({
-      text: 'More specific dashboard',
-      url: '/d/moreSpecificDashboard',
-    });
+  it('handles home page', () => {
+    const mockPage: NavModelItem = {
+      text: 'Something else',
+      id: 'not-home',
+    };
+    expect(getActiveItem(mockNavTree, mockPage, '/')?.id).toEqual('home');
   });
 });

public/app/core/components/AppChrome/MegaMenu/utils.ts

@@ -1,6 +1,7 @@
-import { locationUtil, NavModelItem } from '@grafana/data';
+import { NavModelItem } from '@grafana/data';
 import { config, reportInteraction } from '@grafana/runtime';
 import { t } from 'app/core/internationalization';
+import { HOME_NAV_ID } from 'app/core/reducers/navModel';
 
 import { ShowModalReactEvent } from '../../../../types/events';
 import appEvents from '../../../app_events';
@@ -46,10 +47,6 @@ export const enrichWithInteractionTracking = (item: NavModelItem, megaMenuDocked
   return newItem;
 };
 
-export const isMatchOrChildMatch = (itemToCheck: NavModelItem, searchItem?: NavModelItem) => {
-  return Boolean(itemToCheck === searchItem || hasChildMatch(itemToCheck, searchItem));
-};
-
 export const hasChildMatch = (itemToCheck: NavModelItem, searchItem?: NavModelItem): boolean => {
   return Boolean(
     itemToCheck.children?.some((child) => {
@@ -62,57 +59,48 @@ export const hasChildMatch = (itemToCheck: NavModelItem, searchItem?: NavModelIt
   );
 };
 
-const stripQueryParams = (url?: string) => {
-  return url?.split('?')[0] ?? '';
-};
-
-const isBetterMatch = (newMatch: NavModelItem, currentMatch?: NavModelItem) => {
-  const currentMatchUrl = stripQueryParams(currentMatch?.url);
-  const newMatchUrl = stripQueryParams(newMatch.url);
-  return newMatchUrl && newMatchUrl.length > currentMatchUrl?.length;
-};
-
 export const getActiveItem = (
   navTree: NavModelItem[],
-  pathname: string,
-  currentBestMatch?: NavModelItem
+  currentPage: NavModelItem,
+  url?: string
 ): NavModelItem | undefined => {
-  const dashboardLinkMatch = '/dashboards';
+  const { id, parentItem } = currentPage;
 
-  for (const link of navTree) {
-    const linkWithoutParams = stripQueryParams(link.url);
-    const linkPathname = locationUtil.stripBaseFromUrl(linkWithoutParams);
-    if (linkPathname && link.id !== 'starred') {
-      if (linkPathname === pathname) {
-        // exact match
-        currentBestMatch = link;
-        break;
-      } else if (linkPathname !== '/' && pathname.startsWith(linkPathname)) {
-        // partial match
-        if (isBetterMatch(link, currentBestMatch)) {
-          currentBestMatch = link;
-        }
-      } else if (linkPathname === '/alerting/list' && pathname.startsWith('/alerting/notification/')) {
-        // alert channel match
-        // TODO refactor routes such that we don't need this custom logic
-        currentBestMatch = link;
-        break;
-      } else if (linkPathname === dashboardLinkMatch && pathname.startsWith('/d/')) {
-        // dashboard match
-        // TODO refactor routes such that we don't need this custom logic
-        if (isBetterMatch(link, currentBestMatch)) {
-          currentBestMatch = link;
-        }
+  // special case for the home page
+  if (url === '/') {
+    return navTree.find((item) => item.id === HOME_NAV_ID);
+  }
+
+  // special case for profile as it's not part of the mega menu
+  if (currentPage.id === 'profile') {
+    return undefined;
+  }
+
+  for (const navItem of navTree) {
+    const isIdMatch = Boolean(navItem.id && navItem.id === id);
+    const isTextUrlMatch = navItem.text === currentPage.text && navItem.url === currentPage.url;
+
+    // ideally, we should only match on id
+    // unfortunately it's not a required property of the interface, and there are some cases
+    // where it's not set, particularly with child pages of plugins
+    // in those cases, we fall back to a text + url match
+    if (isIdMatch || isTextUrlMatch) {
+      return navItem;
+    }
+
+    if (navItem.children) {
+      const childrenMatch = getActiveItem(navItem.children, currentPage);
+      if (childrenMatch) {
+        return childrenMatch;
       }
     }
-    if (link.children) {
-      currentBestMatch = getActiveItem(link.children, pathname, currentBestMatch);
-    }
-    if (stripQueryParams(currentBestMatch?.url) === pathname) {
-      return currentBestMatch;
-    }
   }
-  return currentBestMatch;
+
+  if (parentItem) {
+    return getActiveItem(navTree, parentItem);
+  }
+
+  return undefined;
 };
 
 export function getEditionAndUpdateLinks(): NavModelItem[] {

public/app/core/reducers/navModel.ts

@@ -73,6 +73,8 @@ export const updateNavIndex = createAction<NavModelItem>('navIndex/updateNavInde
 // Since the configuration subtitle includes the organization name, we include this action to update the org name if it changes.
 export const updateConfigurationSubtitle = createAction<string>('navIndex/updateConfigurationSubtitle');
 
+export const removeNavIndex = createAction<string>('navIndex/removeNavIndex');
+
 export const getItemWithNewSubTitle = (item: NavModelItem, subTitle: string): NavModelItem => ({
   ...item,
   parentItem: {
@@ -122,6 +124,8 @@ export const navIndexReducer = (state: NavIndex = initialState, action: AnyActio
       'org-settings': getItemWithNewSubTitle(state['org-settings'], subTitle),
       apikeys: getItemWithNewSubTitle(state.apikeys, subTitle),
     };
+  } else if (removeNavIndex.match(action)) {
+    delete state[action.payload];
   }
 
   return state;

public/app/features/dashboard/components/DashNav/DashNav.tsx

@@ -15,6 +15,7 @@ import {
   ConfirmModal,
   Badge,
 } from '@grafana/ui';
+import { updateNavIndex } from 'app/core/actions';
 import { AppChromeUpdate } from 'app/core/components/AppChrome/AppChromeUpdate';
 import { NavToolbarSeparator } from 'app/core/components/AppChrome/NavToolbar/NavToolbarSeparator';
 import config from 'app/core/config';
@@ -22,7 +23,8 @@ import { useAppNotification } from 'app/core/copy/appNotification';
 import { appEvents } from 'app/core/core';
 import { useBusEvent } from 'app/core/hooks/useBusEvent';
 import { t, Trans } from 'app/core/internationalization';
-import { setStarred } from 'app/core/reducers/navBarTree';
+import { ID_PREFIX, setStarred } from 'app/core/reducers/navBarTree';
+import { removeNavIndex } from 'app/core/reducers/navModel';
 import AddPanelButton from 'app/features/dashboard/components/AddPanelButton/AddPanelButton';
 import { SaveDashboardDrawer } from 'app/features/dashboard/components/SaveDashboard/SaveDashboardDrawer';
 import { getDashboardSrv } from 'app/features/dashboard/services/DashboardSrv';
@@ -30,7 +32,7 @@ import { DashboardModel } from 'app/features/dashboard/state';
 import { DashboardInteractions } from 'app/features/dashboard-scene/utils/interactions';
 import { playlistSrv } from 'app/features/playlist/PlaylistSrv';
 import { updateTimeZoneForSession } from 'app/features/profile/state/reducers';
-import { KioskMode } from 'app/types';
+import { KioskMode, StoreState } from 'app/types';
 import { DashboardMetaChangedEvent, ShowModalReactEvent } from 'app/types/events';
 
 import {
@@ -44,11 +46,17 @@ import { DashNavTimeControls } from './DashNavTimeControls';
 import { ShareButton } from './ShareButton';
 
 const mapDispatchToProps = {
+  removeNavIndex,
   setStarred,
   updateTimeZoneForSession,
+  updateNavIndex,
 };
 
-const connector = connect(null, mapDispatchToProps);
+const mapStateToProps = (state: StoreState) => ({
+  navIndex: state.navIndex,
+});
+
+const connector = connect(mapStateToProps, mapDispatchToProps);
 
 const selectors = e2eSelectors.pages.Dashboard.DashNav;
 
@@ -121,10 +129,26 @@ export const DashNav = React.memo<Props>((props) => {
   const onStarDashboard = () => {
     DashboardInteractions.toolbarFavoritesClick();
     const dashboardSrv = getDashboardSrv();
-    const { dashboard, setStarred } = props;
+    const { dashboard, navIndex, removeNavIndex, setStarred, updateNavIndex } = props;
 
     dashboardSrv.starDashboard(dashboard.uid, Boolean(dashboard.meta.isStarred)).then((newState) => {
       setStarred({ id: dashboard.uid, title: dashboard.title, url: dashboard.meta.url ?? '', isStarred: newState });
+      const starredNavItem = navIndex['starred'];
+      if (newState) {
+        starredNavItem.children?.push({
+          id: ID_PREFIX + dashboard.uid,
+          text: dashboard.title,
+          url: dashboard.meta.url ?? '',
+          parentItem: starredNavItem,
+        });
+      } else {
+        removeNavIndex(ID_PREFIX + dashboard.uid);
+        const indexToRemove = starredNavItem.children?.findIndex((element) => element.id === ID_PREFIX + dashboard.uid);
+        if (indexToRemove) {
+          starredNavItem.children?.splice(indexToRemove, 1);
+        }
+      }
+      updateNavIndex(starredNavItem);
       dashboard.meta.isStarred = newState;
       forceUpdate();
     });

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

@@ -53,7 +53,7 @@ export function DashboardSettings({ dashboard, editview, pageNav, sectionNav }:
   const canSave = dashboard.meta.canSave;
   const location = useLocation();
   const editIndex = getEditIndex(location);
-  const subSectionNav = getSectionNav(pageNav, sectionNav, pages, currentPage, location);
+  const subSectionNav = getSectionNav(pageNav, sectionNav, pages, currentPage, location, dashboard.uid);
   const size = 'sm';
 
   const actions = [
@@ -178,7 +178,8 @@ function getSectionNav(
   sectionNav: NavModel,
   pages: SettingsPage[],
   currentPage: SettingsPage,
-  location: H.Location
+  location: H.Location,
+  dashboardUid: string
 ): NavModel {
   const main: NavModelItem = {
     text: t('dashboard-settings.settings.title', 'Settings'),
@@ -191,7 +192,7 @@ function getSectionNav(
   main.children = pages.map((page) => ({
     text: page.title,
     icon: page.icon,
-    id: page.id,
+    id: `${dashboardUid}/${page.id}`,
     url: locationUtil.getUrlForPartial(location, { editview: page.id, editIndex: null }),
     active: page === currentPage,
     parentItem: main,

public/app/features/dashboard/containers/DashboardPage.tsx

@@ -13,6 +13,7 @@ import { GrafanaContext, GrafanaContextType } from 'app/core/context/GrafanaCont
 import { createErrorNotification } from 'app/core/copy/appNotification';
 import { getKioskMode } from 'app/core/navigation/kiosk';
 import { GrafanaRouteComponentProps } from 'app/core/navigation/types';
+import { ID_PREFIX } from 'app/core/reducers/navBarTree';
 import { getNavModel } from 'app/core/selectors/navModel';
 import { PanelModel } from 'app/features/dashboard/state';
 import { dashboardWatcher } from 'app/features/live/dashboard/dashboardWatcher';
@@ -477,7 +478,11 @@ function updateStatePageNavFromProps(props: Props, state: State): State {
       pageNav.parentItem = pageNav.parentItem;
     }
   } else {
-    sectionNav = getNavModel(props.navIndex, 'dashboards/browse');
+    sectionNav = getNavModel(
+      props.navIndex,
+      ID_PREFIX + dashboard.uid,
+      getNavModel(props.navIndex, 'dashboards/browse')
+    );
   }
 
   if (state.editPanel || state.viewPanel) {

public/app/plugins/panel/dashlist/DashList.tsx

@@ -11,16 +11,18 @@ import {
   urlUtil,
 } from '@grafana/data';
 import { CustomScrollbar, useStyles2, IconButton } from '@grafana/ui';
+import { updateNavIndex } from 'app/core/actions';
 import { getConfig } from 'app/core/config';
 import { appEvents } from 'app/core/core';
 import { useBusEvent } from 'app/core/hooks/useBusEvent';
-import { setStarred } from 'app/core/reducers/navBarTree';
+import { ID_PREFIX, setStarred } from 'app/core/reducers/navBarTree';
+import { removeNavIndex } from 'app/core/reducers/navModel';
 import { getBackendSrv } from 'app/core/services/backend_srv';
 import impressionSrv from 'app/core/services/impression_srv';
 import { getDashboardSrv } from 'app/features/dashboard/services/DashboardSrv';
 import { DashboardSearchItem } from 'app/features/search/types';
 import { VariablesChanged } from 'app/features/variables/types';
-import { useDispatch } from 'app/types';
+import { useDispatch, useSelector } from 'app/types';
 
 import { Options } from './panelcfg.gen';
 import { getStyles } from './styles';
@@ -102,6 +104,7 @@ async function fetchDashboards(options: Options, replaceVars: InterpolateFunctio
 export function DashList(props: PanelProps<Options>) {
   const [dashboards, setDashboards] = useState(new Map<string, Dashboard>());
   const dispatch = useDispatch();
+  const navIndex = useSelector((state) => state.navIndex);
 
   useEffect(() => {
     fetchDashboards(props.options, props.replaceVariables).then((dashes) => {
@@ -119,6 +122,23 @@ export function DashList(props: PanelProps<Options>) {
     updatedDashboards.set(dash?.uid ?? '', { ...dash, isStarred });
     setDashboards(updatedDashboards);
     dispatch(setStarred({ id: uid ?? '', title, url, isStarred }));
+
+    const starredNavItem = navIndex['starred'];
+    if (isStarred) {
+      starredNavItem.children?.push({
+        id: ID_PREFIX + uid,
+        text: title,
+        url: url ?? '',
+        parentItem: starredNavItem,
+      });
+    } else {
+      dispatch(removeNavIndex(ID_PREFIX + uid));
+      const indexToRemove = starredNavItem.children?.findIndex((element) => element.id === ID_PREFIX + uid);
+      if (indexToRemove) {
+        starredNavItem.children?.splice(indexToRemove, 1);
+      }
+    }
+    dispatch(updateNavIndex(starredNavItem));
   };
 
   const [starredDashboards, recentDashboards, searchedDashboards] = useMemo(() => {