PageRegistration / PageMenuRegistration fixes & more simplification (#1386)

* PageRegistration & PageMenuRegistration fixes & more simplification, fix #1383

Signed-off-by: Roman <ixrock@gmail.com>

* clean up test, deprecate page.routePath

Signed-off-by: Roman <ixrock@gmail.com>

* fix: proper `isActive` state matching for page-menu-target to page considering current document location

Signed-off-by: Roman <ixrock@gmail.com>

extensions/support-page/main.ts

@@ -6,7 +6,7 @@ export default class SupportPageMainExtension extends LensMainExtension {
       parentId: "help",
       label: "Support",
       click: () => {
-        this.navigate("/support");
+        this.navigate();
       }
     }
   ]

extensions/support-page/renderer.tsx

@@ -5,8 +5,6 @@ import { SupportPage } from "./src/support";
 export default class SupportPageRendererExtension extends LensRendererExtension {
   globalPages: Interface.PageRegistration[] = [
     {
-      id: "support",
-      routePath: "/support",
       components: {
         Page: SupportPage,
       }
@@ -16,7 +14,7 @@ export default class SupportPageRendererExtension extends LensRendererExtension
   statusBarItems: Interface.StatusBarRegistration[] = [
     {
       item: (
-        <div className="SupportPageIcon flex align-center" onClick={() => this.navigate("/support")}>
+        <div className="SupportPageIcon flex align-center" onClick={() => this.navigate()}>
           <Component.Icon interactive material="help" smallest/>
         </div>
       )

src/extensions/lens-main-extension.ts

@@ -2,14 +2,18 @@ import type { MenuRegistration } from "./registries/menu-registry";
 import { observable } from "mobx";
 import { LensExtension } from "./lens-extension"
 import { WindowManager } from "../main/window-manager";
-import { getPageUrl } from "./registries/page-registry"
+import { getExtensionPageUrl } from "./registries/page-registry"
 
 export class LensMainExtension extends LensExtension {
   @observable.shallow appMenus: MenuRegistration[] = []
 
-  async navigate(location?: string, frameId?: number) {
+  async navigate<P extends object>(pageId?: string, params?: P, frameId?: number) {
     const windowManager = WindowManager.getInstance<WindowManager>();
-    const url = getPageUrl(this, location); // get full path to extension's page
-    await windowManager.navigate(url, frameId);
+    const pageUrl = getExtensionPageUrl({
+      extensionId: this.name,
+      pageId: pageId,
+      params: params ?? {}, // compile to url with params
+    });
+    await windowManager.navigate(pageUrl, frameId);
   }
 }

src/extensions/lens-renderer-extension.ts

@@ -1,7 +1,7 @@
 import type { AppPreferenceRegistration, ClusterFeatureRegistration, KubeObjectDetailRegistration, KubeObjectMenuRegistration, KubeObjectStatusRegistration, PageMenuRegistration, PageRegistration, StatusBarRegistration, } from "./registries"
 import { observable } from "mobx";
 import { LensExtension } from "./lens-extension"
-import { getPageUrl } from "./registries/page-registry"
+import { getExtensionPageUrl } from "./registries/page-registry"
 
 export class LensRendererExtension extends LensExtension {
   @observable.shallow globalPages: PageRegistration[] = []
@@ -15,8 +15,13 @@ export class LensRendererExtension extends LensExtension {
   @observable.shallow kubeObjectDetailItems: KubeObjectDetailRegistration[] = []
   @observable.shallow kubeObjectMenuItems: KubeObjectMenuRegistration[] = []
 
-  async navigate(location?: string) {
+  async navigate<P extends object>(pageId?: string, params?: P) {
     const { navigate } = await import("../renderer/navigation");
-    navigate(getPageUrl(this, location));
+    const pageUrl = getExtensionPageUrl({
+      extensionId: this.name,
+      pageId: pageId,
+      params: params ?? {}, // compile to url with params
+    });
+    navigate(pageUrl);
   }
 }

src/extensions/registries/__tests__/page-registry.test.ts

@@ -1,4 +1,4 @@
-import { getPageUrl, globalPageRegistry } from "../page-registry"
+import { getExtensionPageUrl, globalPageRegistry, PageRegistration } from "../page-registry"
 import { LensExtension } from "../../lens-extension"
 import React from "react";
 
@@ -18,20 +18,19 @@ describe("getPageUrl", () => {
   })
 
   it("returns a page url for extension", () => {
-    expect(getPageUrl(ext)).toBe("/extension/foo-bar")
+    expect(getExtensionPageUrl({ extensionId: ext.name })).toBe("/extension/foo-bar")
   })
 
   it("allows to pass base url as parameter", () => {
-    expect(getPageUrl(ext, "/test")).toBe("/extension/foo-bar/test")
+    expect(getExtensionPageUrl({ extensionId: ext.name, pageId: "/test" })).toBe("/extension/foo-bar/test")
   })
 
   it("removes @", () => {
-    ext.manifest.name = "@foo/bar"
-    expect(getPageUrl(ext)).toBe("/extension/foo-bar")
+    expect(getExtensionPageUrl({ extensionId: "@foo/bar" })).toBe("/extension/foo-bar")
   })
 
   it("adds / prefix", () => {
-    expect(getPageUrl(ext, "test")).toBe("/extension/foo-bar/test")
+    expect(getExtensionPageUrl({ extensionId: ext.name, pageId: "test" })).toBe("/extension/foo-bar/test")
   })
 })
 
@@ -57,12 +56,24 @@ describe("globalPageRegistry", () => {
         id: "another-page",
         components: {
           Page: () => React.createElement('Text')
+        },
+      },
+      {
+        components: {
+          Page: () => React.createElement('Default')
         }
       },
     ], ext)
   })
 
   describe("getByPageMenuTarget", () => {
+    it("matching to first registered page without id", () => {
+      const page = globalPageRegistry.getByPageMenuTarget({ extensionId: ext.name })
+      expect(page.id).toEqual(undefined);
+      expect(page.extensionId).toEqual(ext.name);
+      expect(page.routePath).toEqual(getExtensionPageUrl({ extensionId: ext.name }));
+    });
+
     it("returns matching page", () => {
       const page = globalPageRegistry.getByPageMenuTarget({
         pageId: "test-page",

src/extensions/registries/base-registry.ts

@@ -1,13 +1,15 @@
 // Base class for extensions-api registries
 import { action, observable } from "mobx";
+import { LensExtension } from "../lens-extension";
 
-export class BaseRegistry<T = any> {
+export class BaseRegistry<T = object, I extends T = T> {
   private items = observable<T>([], { deep: false });
 
-  getItems(): T[] {
-    return this.items.toJS();
+  getItems(): I[] {
+    return this.items.toJS() as I[];
   }
 
+  add(items: T | T[], ext?: LensExtension): () => void; // allow method overloading with required "ext"
   @action
   add(items: T | T[]) {
     const normalizedItems = (Array.isArray(items) ? items : [items])

src/extensions/registries/page-menu-registry.ts

@@ -1,15 +1,14 @@
 // Extensions-api -> Register page menu items
-
+import type { IconProps } from "../../renderer/components/icon";
 import type React from "react";
 import { action } from "mobx";
-import type { IconProps } from "../../renderer/components/icon";
 import { BaseRegistry } from "./base-registry";
 import { LensExtension } from "../lens-extension";
 
-export interface PageMenuTarget {
-  pageId: string;
+export interface PageMenuTarget<P extends object = any> {
   extensionId?: string;
-  params?: object;
+  pageId?: string;
+  params?: P;
 }
 
 export interface PageMenuRegistration {
@@ -22,19 +21,19 @@ export interface PageMenuComponents {
   Icon: React.ComponentType<IconProps>;
 }
 
-export class PageMenuRegistry<T extends PageMenuRegistration> extends BaseRegistry<T> {
-
+export class PageMenuRegistry extends BaseRegistry<PageMenuRegistration, Required<PageMenuRegistration>> {
   @action
-  add(items: T[], ext?: LensExtension) {
-    const normalizedItems = items.map((menu) => {
-      if (menu.target && !menu.target.extensionId) {
-        menu.target.extensionId = ext.name
-      }
-      return menu
+  add(items: PageMenuRegistration[], ext: LensExtension) {
+    const normalizedItems = items.map(menuItem => {
+      menuItem.target = {
+        extensionId: ext.name,
+        ...(menuItem.target || {}),
+      };
+      return menuItem
     })
     return super.add(normalizedItems);
   }
 }
 
-export const globalPageMenuRegistry = new PageMenuRegistry<PageMenuRegistration>();
-export const clusterPageMenuRegistry = new PageMenuRegistry<PageMenuRegistration>();
+export const globalPageMenuRegistry = new PageMenuRegistry();
+export const clusterPageMenuRegistry = new PageMenuRegistry();

src/extensions/registries/page-registry.ts

@@ -1,59 +1,96 @@
 // Extensions-api -> Custom page registration
-
-import React from "react";
+import type { PageMenuTarget } from "./page-menu-registry";
+import type React from "react";
+import path from "path";
 import { action } from "mobx";
 import { compile } from "path-to-regexp";
 import { BaseRegistry } from "./base-registry";
-import { LensExtension } from "../lens-extension"
-import type { PageMenuTarget } from "./page-menu-registry";
+import { LensExtension } from "../lens-extension";
+import logger from "../../main/logger";
 
 export interface PageRegistration {
-  id: string; // will be automatically prefixed with extension name
-  routePath?: string; // additional (suffix) route path to base extension's route: "/extension/:name"
-  exact?: boolean; // route matching flag, see: https://reactrouter.com/web/api/NavLink/exact-bool
+  /**
+   * Page ID or additional route path to indicate uniqueness within current extension registered pages
+   * Might contain special url placeholders, e.g. "/users/:userId?" (? - marks as optional param)
+   * When not provided, first registered page without "id" would be used for page-menus without target.pageId for same extension
+   */
+  id?: string;
+  /**
+   * Alias to page ID which assume to be used as path with possible :param placeholders
+   * @deprecated
+   */
+  routePath?: string;
+  /**
+   * Strict route matching to provided page-id, read also: https://reactrouter.com/web/api/NavLink/exact-bool
+   * In case when more than one page registered at same extension "pageId" is required to identify different pages,
+   * It might be useful to provide `exact: true` in some cases to avoid overlapping routes.
+   * Without {exact:true} second page never matches since first page-id/route already includes partial route.
+   * @example const pages = [
+   *  {id: "/users", exact: true},
+   *  {id: "/users/:userId?"}
+   * ]
+   * Pro-tip: registering pages in opposite order will make same effect without "exact".
+   */
+  exact?: boolean;
   components: PageComponents;
 }
 
+export interface RegisteredPage extends PageRegistration {
+  extensionId: string; // required for compiling registered page to url with page-menu-target to compare
+  routePath: string; // full route-path to registered extension page
+}
+
 export interface PageComponents {
   Page: React.ComponentType<any>;
 }
 
-const routePrefix = "/extension/:name"
-
-export function sanitizeExtensioName(name: string) {
+export function sanitizeExtensionName(name: string) {
   return name.replace("@", "").replace("/", "-")
 }
 
-export function getPageUrl(ext: LensExtension, baseUrl = "") {
-  if (baseUrl !== "" && !baseUrl.startsWith("/")) {
-    baseUrl = "/" + baseUrl
+export function getExtensionPageUrl<P extends object>({ extensionId, pageId = "", params }: PageMenuTarget<P>): string {
+  const extensionBaseUrl = compile(`/extension/:name`)({
+    name: sanitizeExtensionName(extensionId), // compile only with extension-id first and define base path
+  });
+  const extPageRoutePath = path.join(extensionBaseUrl, pageId); // page-id might contain route :param-s, so don't compile yet
+  if (params) {
+    return compile(extPageRoutePath)(params); // might throw error when required params not passed
   }
-  const validUrlName = sanitizeExtensioName(ext.name);
-  return compile(routePrefix)({ name: validUrlName }) + baseUrl;
+  return extPageRoutePath;
 }
 
-export class PageRegistry<T extends PageRegistration> extends BaseRegistry<T> {
-
+export class PageRegistry extends BaseRegistry<PageRegistration, RegisteredPage> {
   @action
-  add(items: T[], ext?: LensExtension) {
-    const normalizedItems = items.map((page) => {
-      if (!page.routePath) {
-        page.routePath = `/${page.id}`
-      }
-      page.routePath = getPageUrl(ext, page.routePath)
-      return page
+  add(items: PageRegistration[], ext: LensExtension) {
+    let registeredPages: RegisteredPage[] = [];
+    try {
+      registeredPages = items.map(page => ({
+        ...page,
+        extensionId: ext.name,
+        routePath: getExtensionPageUrl({ extensionId: ext.name, pageId: page.id ?? page.routePath }),
+      }))
+    } catch (err) {
+      logger.error(`[EXTENSION]: page-registration failed`, {
+        items,
+        extension: ext,
+        error: String(err),
       })
-    return super.add(normalizedItems);
+    }
+    return super.add(registeredPages);
   }
 
-  getByPageMenuTarget(target: PageMenuTarget) {
-    if (!target) {
-      return null
+  getUrl<P extends object>({ extensionId, id: pageId }: RegisteredPage, params?: P) {
+    return getExtensionPageUrl({ extensionId, pageId, params });
   }
-    const routePath = `/extension/${sanitizeExtensioName(target.extensionId)}/`
-    return this.getItems().find((page) => page.routePath.startsWith(routePath) && page.id === target.pageId) || null
+
+  getByPageMenuTarget(target: PageMenuTarget = {}): RegisteredPage | null {
+    const targetUrl = getExtensionPageUrl(target);
+    return this.getItems().find(({ id: pageId, extensionId }) => {
+      const pageUrl = getExtensionPageUrl({ extensionId, pageId, params: target.params }); // compiled with provided params
+      return targetUrl === pageUrl;
+    }) || null;
   }
 }
 
-export const globalPageRegistry = new PageRegistry<PageRegistration>();
-export const clusterPageRegistry = new PageRegistry<PageRegistration>();
+export const globalPageRegistry = new PageRegistry();
+export const clusterPageRegistry = new PageRegistry();

src/renderer/components/app.tsx

@@ -29,7 +29,6 @@ import { CustomResources } from "./+custom-resources/custom-resources";
 import { crdRoute } from "./+custom-resources";
 import { isAllowedResource } from "../../common/rbac";
 import { MainLayout } from "./layout/main-layout";
-import { TabLayout, TabLayoutRoute } from "./layout/tab-layout";
 import { ErrorBoundary } from "./error-boundary";
 import { Terminal } from "./dock/terminal";
 import { getHostedCluster, getHostedClusterId } from "../../common/cluster-store";
@@ -37,7 +36,6 @@ import logger from "../../main/logger";
 import { clusterIpc } from "../../common/cluster-ipc";
 import { webFrame } from "electron";
 import { clusterPageRegistry } from "../../extensions/registries/page-registry";
-import { clusterPageMenuRegistry } from "../../extensions/registries";
 import { extensionLoader } from "../../extensions/extension-loader";
 import { appEventBus } from "../../common/event-bus";
 import whatInput from 'what-input';
@@ -75,10 +73,7 @@ export class App extends React.Component {
 
   renderExtensionRoutes() {
     return clusterPageRegistry.getItems().map(({ components: { Page }, exact, routePath }) => {
-      const Component = () => {
-        return <Page/>
-      };
-      return <Route key={routePath} path={routePath} exact={exact} component={Component}/>
+      return <Route key={routePath} path={routePath} exact={exact} component={Page}/>
     })
   }
 

src/renderer/components/cluster-manager/clusters-menu.tsx

@@ -14,7 +14,7 @@ import { ClusterIcon } from "../cluster-icon";
 import { Icon } from "../icon";
 import { autobind, cssNames, IClassName } from "../../utils";
 import { Badge } from "../badge";
-import { isActiveRoute, navigate } from "../../navigation";
+import { navigate, navigation } from "../../navigation";
 import { addClusterURL } from "../+add-cluster";
 import { clusterSettingsURL } from "../+cluster-settings";
 import { landingURL } from "../+landing-page";
@@ -22,8 +22,7 @@ import { Tooltip } from "../tooltip";
 import { ConfirmDialog } from "../confirm-dialog";
 import { clusterIpc } from "../../../common/cluster-ipc";
 import { clusterViewURL } from "./cluster-view.route";
-import { globalPageMenuRegistry, globalPageRegistry } from "../../../extensions/registries";
-import { compile } from "path-to-regexp";
+import { getExtensionPageUrl, globalPageMenuRegistry, globalPageRegistry } from "../../../extensions/registries";
 
 interface Props {
   className?: IClassName;
@@ -152,13 +151,15 @@ export class ClustersMenu extends React.Component<Props> {
           {globalPageMenuRegistry.getItems().map(({ title, target, components: { Icon } }) => {
             const registeredPage = globalPageRegistry.getByPageMenuTarget(target);
             if (!registeredPage) return;
-            const { routePath, exact } = registeredPage;
+            const { extensionId, id: pageId } = registeredPage;
+            const pageUrl = getExtensionPageUrl({ extensionId, pageId, params: target.params });
+            const isActive = pageUrl === navigation.location.pathname;
             return (
               <Icon
-                key={routePath}
+                key={pageUrl}
                 tooltip={title}
-                active={isActiveRoute({ path: routePath, exact })}
-                onClick={() => navigate(compile(routePath)(target.params))}
+                active={isActive}
+                onClick={() => navigate(pageUrl)}
               />
             )
           })}

src/renderer/components/layout/sidebar.tsx

@@ -26,11 +26,10 @@ import { Network } from "../+network";
 import { crdStore } from "../+custom-resources/crd.store";
 import { CrdList, crdResourcesRoute, crdRoute, crdURL } from "../+custom-resources";
 import { CustomResources } from "../+custom-resources/custom-resources";
-import { isActiveRoute } from "../../navigation";
+import { isActiveRoute, navigation } from "../../navigation";
 import { isAllowedResource } from "../../../common/rbac"
 import { Spinner } from "../spinner";
-import { clusterPageMenuRegistry, clusterPageRegistry } from "../../../extensions/registries";
-import { compile } from "path-to-regexp";
+import { clusterPageMenuRegistry, clusterPageRegistry, getExtensionPageUrl } from "../../../extensions/registries";
 
 const SidebarContext = React.createContext<SidebarContextValue>({ pinned: false });
 type SidebarContextValue = {
@@ -195,15 +194,14 @@ export class Sidebar extends React.Component<Props> {
             {clusterPageMenuRegistry.getItems().map(({ title, target, components: { Icon } }) => {
               const registeredPage = clusterPageRegistry.getByPageMenuTarget(target);
               if (!registeredPage) return;
-              const { routePath, exact } = registeredPage;
-              const url = compile(routePath)(target.params)
+              const { extensionId, id: pageId } = registeredPage;
+              const pageUrl = getExtensionPageUrl({ extensionId, pageId, params: target.params });
+              const isActive = pageUrl === navigation.location.pathname;
               return (
                 <SidebarNavItem
-                  key={url}
-                  url={url}
-                  text={title}
-                  icon={<Icon/>}
-                  isActive={isActiveRoute({ path: routePath, exact })}
+                  key={pageUrl} url={pageUrl}
+                  text={title} icon={<Icon/>}
+                  isActive={isActive}
                 />
               )
             })}

src/renderer/navigation.ts

@@ -22,8 +22,12 @@ export function navigate(location: LocationDescriptor) {
   }
 }
 
+export function matchParams<P>(route: string | string[] | RouteProps) {
+  return matchPath<P>(navigation.location.pathname, route);
+}
+
 export function isActiveRoute(route: string | string[] | RouteProps): boolean {
-  return !!matchPath(navigation.location.pathname, route);
+  return !!matchParams(route);
 }
 
 // common params for all pages