TopNav: Fixes to page and plugin page handling (#56278)

* Added labels

* App page fixes

* Switch to switch

* Fixing sort position for Apps

packages/grafana-runtime/src/components/PluginPage.tsx

@@ -3,6 +3,8 @@ import React from 'react';
 import { NavModelItem, PageLayoutType } from '@grafana/data';
 
 export interface PluginPageProps {
+  /** Shown under main heading */
+  subTitle?: React.ReactNode;
   pageNav?: NavModelItem;
   children: React.ReactNode;
   layout?: PageLayoutType;

pkg/services/navtree/models.go

@@ -20,6 +20,9 @@ const (
 	WeightDataConnections
 	WeightPlugin
 	WeightConfig
+	WeightAlertsAndIncidents
+	WeightMonitoring
+	WeightApps
 	WeightAdmin
 	WeightProfile
 	WeightHelp
@@ -40,6 +43,7 @@ const (
 	NavIDAlerting           = "alerting"
 	NavIDMonitoring         = "monitoring"
 	NavIDReporting          = "reports"
+	NavIDApps               = "apps"
 )
 
 type NavLink struct {
@@ -112,7 +116,7 @@ func (root *NavTreeRoot) RemoveEmptySectionsAndApplyNewInformationArchitecture(t
 		if serverAdminNode := root.FindById(NavIDAdmin); serverAdminNode != nil {
 			serverAdminNode.Url = "/admin/settings"
 			serverAdminNode.Text = "Server admin"
-			serverAdminNode.SortWeight = 10000
+			serverAdminNode.SortWeight = 0
 
 			if orgAdminNode != nil {
 				orgAdminNode.Children = append(orgAdminNode.Children, serverAdminNode)

pkg/services/navtree/navtreeimpl/applinks.go

@@ -57,20 +57,8 @@ func (s *ServiceImpl) addAppLinks(treeRoot *navtree.NavTreeRoot, c *models.ReqCo
 		})
 	}
 
-	if topNavEnabled {
-		treeRoot.AddSection(&navtree.NavLink{
-			Text:     "Apps",
-			Icon:     "apps",
-			SubTitle: "App plugins that extend the Grafana experience",
-			Id:       "apps",
-			Children: appLinks,
-			Section:  navtree.NavSectionCore,
-			Url:      s.cfg.AppSubURL + "/apps",
-		})
-	} else {
-		for _, appLink := range appLinks {
-			treeRoot.AddSection(appLink)
-		}
+	for _, appLink := range appLinks {
+		treeRoot.AddSection(appLink)
 	}
 
 	return nil
@@ -134,47 +122,79 @@ func (s *ServiceImpl) processAppPlugin(plugin plugins.PluginDTO, c *models.ReqCo
 		}
 	}
 
-	if len(appLink.Children) > 0 {
-		// If we only have one child and it's the app default nav then remove it from children
-		if len(appLink.Children) == 1 && appLink.Children[0].Url == appLink.Url {
-			appLink.Children = []*navtree.NavLink{}
+	// Apps without any nav children are not part of navtree
+	if len(appLink.Children) == 0 {
+		return nil
+	}
+	// If we only have one child and it's the app default nav then remove it from children
+	if len(appLink.Children) == 1 && appLink.Children[0].Url == appLink.Url {
+		appLink.Children = []*navtree.NavLink{}
+	}
+
+	if !topNavEnabled {
+		return appLink
+	}
+
+	// Remove default nav child
+	childrenWithoutDefault := []*navtree.NavLink{}
+	for _, child := range appLink.Children {
+		if child.Url != appLink.Url {
+			childrenWithoutDefault = append(childrenWithoutDefault, child)
 		}
+	}
+	appLink.Children = childrenWithoutDefault
 
-		alertingNode := treeRoot.FindById(navtree.NavIDAlerting)
+	// Handle moving apps into specific navtree sections
+	alertingNode := treeRoot.FindById(navtree.NavIDAlerting)
+	sectionID := "apps"
 
-		if navConfig, hasOverride := s.navigationAppConfig[plugin.ID]; hasOverride && topNavEnabled {
-			appLink.SortWeight = navConfig.SortWeight
+	if navConfig, hasOverride := s.navigationAppConfig[plugin.ID]; hasOverride {
+		appLink.SortWeight = navConfig.SortWeight
+		sectionID = navConfig.SectionID
+	}
 
-			if navNode := treeRoot.FindById(navConfig.SectionID); navNode != nil {
-				navNode.Children = append(navNode.Children, appLink)
-			} else {
-				if navConfig.SectionID == navtree.NavIDMonitoring {
-					treeRoot.AddSection(&navtree.NavLink{
-						Text:     "Monitoring",
-						Id:       navtree.NavIDMonitoring,
-						SubTitle: "Monitoring and infrastructure apps",
-						Icon:     "heart-rate",
-						Section:  navtree.NavSectionCore,
-						Children: []*navtree.NavLink{appLink},
-						Url:      s.cfg.AppSubURL + "/monitoring",
-					})
-				} else if navConfig.SectionID == navtree.NavIDAlertsAndIncidents && alertingNode != nil {
-					treeRoot.AddSection(&navtree.NavLink{
-						Text:     "Alerts & incidents",
-						Id:       navtree.NavIDAlertsAndIncidents,
-						SubTitle: "Alerting and incident management apps",
-						Icon:     "bell",
-						Section:  navtree.NavSectionCore,
-						Children: []*navtree.NavLink{alertingNode, appLink},
-						Url:      s.cfg.AppSubURL + "/alerts-and-incidents",
-					})
-					treeRoot.RemoveSection(alertingNode)
-				} else {
-					s.log.Error("Plugin app nav id not found", "pluginId", plugin.ID, "navId", navConfig.SectionID)
-				}
+	if navNode := treeRoot.FindById(sectionID); navNode != nil {
+		navNode.Children = append(navNode.Children, appLink)
+	} else {
+		switch sectionID {
+		case navtree.NavIDApps:
+			treeRoot.AddSection(&navtree.NavLink{
+				Text:       "Apps",
+				Icon:       "apps",
+				SubTitle:   "App plugins that extend the Grafana experience",
+				Id:         navtree.NavIDApps,
+				Children:   []*navtree.NavLink{appLink},
+				Section:    navtree.NavSectionCore,
+				SortWeight: navtree.WeightApps,
+				Url:        s.cfg.AppSubURL + "/apps",
+			})
+		case navtree.NavIDMonitoring:
+			treeRoot.AddSection(&navtree.NavLink{
+				Text:       "Monitoring",
+				Id:         navtree.NavIDMonitoring,
+				SubTitle:   "Monitoring and infrastructure apps",
+				Icon:       "heart-rate",
+				Section:    navtree.NavSectionCore,
+				SortWeight: navtree.WeightMonitoring,
+				Children:   []*navtree.NavLink{appLink},
+				Url:        s.cfg.AppSubURL + "/monitoring",
+			})
+		case navtree.NavIDAlertsAndIncidents:
+			if alertingNode != nil {
+				treeRoot.AddSection(&navtree.NavLink{
+					Text:       "Alerts & incidents",
+					Id:         navtree.NavIDAlertsAndIncidents,
+					SubTitle:   "Alerting and incident management apps",
+					Icon:       "bell",
+					Section:    navtree.NavSectionCore,
+					SortWeight: navtree.WeightAlertsAndIncidents,
+					Children:   []*navtree.NavLink{alertingNode, appLink},
+					Url:        s.cfg.AppSubURL + "/alerts-and-incidents",
+				})
+				treeRoot.RemoveSection(alertingNode)
 			}
-		} else {
-			return appLink
+		default:
+			s.log.Error("Plugin app nav id not found", "pluginId", plugin.ID, "navId", sectionID)
 		}
 	}
 

pkg/services/navtree/navtreeimpl/applinks_test.go

@@ -32,14 +32,14 @@ func TestAddAppLinks(t *testing.T) {
 			Type: plugins.App,
 			Includes: []*plugins.Includes{
 				{
-					Name:       "Hello",
+					Name:       "Catalog",
 					Path:       "/a/test-app1/catalog",
 					Type:       "page",
 					AddToNav:   true,
 					DefaultNav: true,
 				},
 				{
-					Name:     "Hello",
+					Name:     "Page2",
 					Path:     "/a/test-app1/page2",
 					Type:     "page",
 					AddToNav: true,
@@ -99,6 +99,16 @@ func TestAddAppLinks(t *testing.T) {
 		require.Equal(t, "Test app1 name", treeRoot.Children[0].Children[0].Text)
 	})
 
+	t.Run("Should remove add default nav child when topnav is enabled", func(t *testing.T) {
+		service.features = featuremgmt.WithFeatures(featuremgmt.FlagTopnav)
+		treeRoot := navtree.NavTreeRoot{}
+		err := service.addAppLinks(&treeRoot, reqCtx)
+		require.NoError(t, err)
+		require.Equal(t, "Apps", treeRoot.Children[0].Text)
+		require.Equal(t, "Test app1 name", treeRoot.Children[0].Children[0].Text)
+		require.Equal(t, "Page2", treeRoot.Children[0].Children[0].Children[0].Text)
+	})
+
 	t.Run("Should move apps that have specific nav id configured to correct section", func(t *testing.T) {
 		service.features = featuremgmt.WithFeatures(featuremgmt.FlagTopnav)
 		service.navigationAppConfig = map[string]NavigationAppConfig{

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

@@ -96,7 +96,7 @@ export const Page: PageType = ({
       )}
       {layout === PageLayoutType.Canvas && (
         <CustomScrollbar autoHeightMin={'100%'} scrollTop={scrollTop} scrollRefCallback={scrollRef}>
-          <div className={styles.dashboardContent}>
+          <div className={styles.canvasContent}>
             {toolbar}
             {children}
           </div>
@@ -137,14 +137,16 @@ const getStyles = (theme: GrafanaTheme2) => {
         top: theme.spacing(2),
       },
     }),
-    wrapper: css`
-      height: 100%;
-      display: flex;
-      flex: 1 1 0;
-      flex-direction: column;
-      min-height: 0;
-    `,
+    wrapper: css({
+      label: 'page-wrapper',
+      height: '100%',
+      display: 'flex',
+      flex: '1 1 0',
+      flexDirection: 'column',
+      minHeight: 0,
+    }),
     panes: css({
+      label: 'page-panes',
       display: 'flex',
       height: '100%',
       width: '100%',
@@ -156,18 +158,19 @@ const getStyles = (theme: GrafanaTheme2) => {
       },
     }),
     pageContent: css({
+      label: 'page-content',
       flexGrow: 1,
     }),
     pageInner: css({
+      label: 'page-inner',
       padding: theme.spacing(3),
       boxShadow: shadow,
       background: theme.colors.background.primary,
       margin: theme.spacing(2, 2, 2, 1),
-      display: 'flex',
-      flexDirection: 'column',
       flexGrow: 1,
     }),
-    dashboardContent: css({
+    canvasContent: css({
+      label: 'canvas-content',
       display: 'flex',
       flexDirection: 'column',
       padding: theme.spacing(2),

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

@@ -5,11 +5,11 @@ import { PluginPageContext } from 'app/features/plugins/components/PluginPageCon
 
 import { Page } from '../Page/Page';
 
-export function PluginPage({ children, pageNav, layout }: PluginPageProps) {
+export function PluginPage({ children, pageNav, layout, subTitle }: PluginPageProps) {
   const context = useContext(PluginPageContext);
 
   return (
-    <Page navModel={context.sectionNav} pageNav={pageNav} layout={layout}>
+    <Page navModel={context.sectionNav} pageNav={pageNav} layout={layout} subTitle={subTitle}>
       <Page.Contents>{children}</Page.Contents>
     </Page>
   );

public/app/features/plugins/utils.test.ts

@@ -10,6 +10,7 @@ describe('buildPluginSectionNav', () => {
   const app1: NavModelItem = {
     text: 'App1',
     id: 'plugin-page-app1',
+    url: '/a/plugin1',
     children: [
       {
         text: 'page1',
@@ -74,6 +75,18 @@ describe('buildPluginSectionNav', () => {
     expect(result?.node.text).toBe('page2');
   });
 
+  it('Should set app section to active', () => {
+    config.featureToggles.topnav = true;
+    const result = buildPluginSectionNav(
+      { pathname: '/a/plugin1', search: '' } as HistoryLocation,
+      null,
+      navIndex,
+      'app1'
+    );
+    expect(result?.main.children![0].active).toBe(true);
+    expect(result?.node.text).toBe('App1');
+  });
+
   it('Should handle standalone page', () => {
     config.featureToggles.topnav = true;
     const result = buildPluginSectionNav(

public/app/features/plugins/utils.ts

@@ -76,7 +76,7 @@ export function buildPluginSectionNav(
   section.children = (section?.children ?? []).map((child) => {
     if (child.children) {
       return {
-        ...child,
+        ...setPageToActive(child, currentUrl),
         children: child.children.map((pluginPage) => setPageToActive(pluginPage, currentUrl)),
       };
     }