CommandPalette: Render dashboard + nav actions as links (#62315)

* CommandPalette: Render links as links!

* Update public/app/features/commandPalette/KBarResults.tsx

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

* Apply suggestions from code review

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

* fix ellipsis showing

---------

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

package.json

@@ -391,6 +391,7 @@
     "react-table": "7.8.0",
     "react-transition-group": "4.4.5",
     "react-use": "17.4.0",
+    "react-virtual": "2.8.2",
     "react-virtualized-auto-sizer": "1.0.7",
     "react-window": "1.8.8",
     "react-window-infinite-loader": "1.0.8",

public/app/features/commandPalette/CommandPalette.tsx

@@ -6,7 +6,6 @@ import {
   KBarAnimator,
   KBarPortal,
   KBarPositioner,
-  KBarResults,
   KBarSearch,
   VisualState,
   useRegisterActions,
@@ -20,6 +19,7 @@ import { config, reportInteraction } from '@grafana/runtime';
 import { Icon, Spinner, useStyles2 } from '@grafana/ui';
 import { t } from 'app/core/internationalization';
 
+import { KBarResults } from './KBarResults';
 import { ResultItem } from './ResultItem';
 import { useDashboardResults } from './actions/dashboardActions';
 import useActions from './actions/useActions';

public/app/features/commandPalette/KBarResults.tsx

@@ -0,0 +1,226 @@
+import { ActionImpl, getListboxItemId, KBAR_LISTBOX, useKBar } from 'kbar';
+import { usePointerMovedSinceMount } from 'kbar/lib/utils';
+import * as React from 'react';
+import { useVirtual } from 'react-virtual';
+
+// From https://github.com/timc1/kbar/blob/main/src/KBarResults.tsx
+// TODO: Go back to KBarResults from kbar when https://github.com/timc1/kbar/issues/281 is fixed
+// Remember to remove dependency on react-virtual when removing this file
+
+const START_INDEX = 0;
+
+interface RenderParams<T = ActionImpl | string> {
+  item: T;
+  active: boolean;
+}
+
+interface KBarResultsProps {
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  items: any[];
+  onRender: (params: RenderParams) => React.ReactElement;
+  maxHeight?: number;
+}
+
+export const KBarResults: React.FC<KBarResultsProps> = (props) => {
+  const activeRef = React.useRef<HTMLElement>(null);
+  const parentRef = React.useRef(null);
+
+  // store a ref to all items so we do not have to pass
+  // them as a dependency when setting up event listeners.
+  const itemsRef = React.useRef(props.items);
+  itemsRef.current = props.items;
+
+  const rowVirtualizer = useVirtual({
+    size: itemsRef.current.length,
+    parentRef,
+  });
+
+  const { query, search, currentRootActionId, activeIndex, options } = useKBar((state) => ({
+    search: state.searchQuery,
+    currentRootActionId: state.currentRootActionId,
+    activeIndex: state.activeIndex,
+  }));
+
+  React.useEffect(() => {
+    const handler = (event: KeyboardEvent) => {
+      if (event.key === 'ArrowUp' || (event.ctrlKey && event.key === 'p')) {
+        event.preventDefault();
+        query.setActiveIndex((index) => {
+          let nextIndex = index > START_INDEX ? index - 1 : index;
+          // avoid setting active index on a group
+          if (typeof itemsRef.current[nextIndex] === 'string') {
+            if (nextIndex === 0) {
+              return index;
+            }
+            nextIndex -= 1;
+          }
+          return nextIndex;
+        });
+      } else if (event.key === 'ArrowDown' || (event.ctrlKey && event.key === 'n')) {
+        event.preventDefault();
+        query.setActiveIndex((index) => {
+          let nextIndex = index < itemsRef.current.length - 1 ? index + 1 : index;
+          // avoid setting active index on a group
+          if (typeof itemsRef.current[nextIndex] === 'string') {
+            if (nextIndex === itemsRef.current.length - 1) {
+              return index;
+            }
+            nextIndex += 1;
+          }
+          return nextIndex;
+        });
+      } else if (event.key === 'Enter') {
+        event.preventDefault();
+        // storing the active dom element in a ref prevents us from
+        // having to calculate the current action to perform based
+        // on the `activeIndex`, which we would have needed to add
+        // as part of the dependencies array.
+        activeRef.current?.click();
+      }
+    };
+    window.addEventListener('keydown', handler);
+    return () => window.removeEventListener('keydown', handler);
+  }, [query]);
+
+  // destructuring here to prevent linter warning to pass
+  // entire rowVirtualizer in the dependencies array.
+  const { scrollToIndex } = rowVirtualizer;
+  React.useEffect(() => {
+    scrollToIndex(activeIndex, {
+      // ensure that if the first item in the list is a group
+      // name and we are focused on the second item, to not
+      // scroll past that group, hiding it.
+      align: activeIndex <= 1 ? 'end' : 'auto',
+    });
+  }, [activeIndex, scrollToIndex]);
+
+  React.useEffect(() => {
+    // TODO(tim): fix scenario where async actions load in
+    // and active index is reset to the first item. i.e. when
+    // users register actions and bust the `useRegisterActions`
+    // cache, we won't want to reset their active index as they
+    // are navigating the list.
+    query.setActiveIndex(
+      // avoid setting active index on a group
+      typeof props.items[START_INDEX] === 'string' ? START_INDEX + 1 : START_INDEX
+    );
+  }, [search, currentRootActionId, props.items, query]);
+
+  const execute = React.useCallback(
+    (ev: React.MouseEvent, item: RenderParams['item']) => {
+      if (typeof item === 'string') {
+        return;
+      }
+
+      // ActionImpl constructor copies all properties from action onto ActionImpl
+      // so our url property is secretly there, but completely untyped
+      // Preferably this change is upstreamed and ActionImpl has this
+      // eslint-disable-next-line
+      const url = (item as ActionImpl & { url?: string }).url;
+
+      if (item.command) {
+        item.command.perform(item);
+        query.toggle();
+      } else if (url) {
+        if (!(ev.ctrlKey || ev.metaKey || ev.shiftKey)) {
+          query.toggle();
+        }
+      } else {
+        query.setSearch('');
+        query.setCurrentRootAction(item.id);
+      }
+
+      options.callbacks?.onSelectAction?.(item);
+    },
+    [query, options]
+  );
+
+  const pointerMoved = usePointerMovedSinceMount();
+
+  return (
+    <div
+      ref={parentRef}
+      style={{
+        maxHeight: props.maxHeight || 400,
+        position: 'relative',
+        overflow: 'auto',
+      }}
+    >
+      <div
+        role="listbox"
+        id={KBAR_LISTBOX}
+        style={{
+          height: `${rowVirtualizer.totalSize}px`,
+          width: '100%',
+        }}
+      >
+        {rowVirtualizer.virtualItems.map((virtualRow) => {
+          const item = itemsRef.current[virtualRow.index];
+
+          // ActionImpl constructor copies all properties from action onto ActionImpl
+          // so our url property is secretly there, but completely untyped
+          // Preferably this change is upstreamed and ActionImpl has this
+          // eslint-disable-next-line @typescript-eslint/consistent-type-assertions
+          const url = (item as ActionImpl & { url?: string }).url;
+
+          const handlers = typeof item !== 'string' && {
+            onPointerMove: () =>
+              pointerMoved && activeIndex !== virtualRow.index && query.setActiveIndex(virtualRow.index),
+            onPointerDown: () => query.setActiveIndex(virtualRow.index),
+            onClick: (ev: React.MouseEvent) => execute(ev, item),
+          };
+          const active = virtualRow.index === activeIndex;
+
+          const childProps = {
+            id: getListboxItemId(virtualRow.index),
+            role: 'option',
+            'aria-selected': active,
+            style: {
+              position: 'absolute',
+              top: 0,
+              left: 0,
+              width: '100%',
+              transform: `translateY(${virtualRow.start}px)`,
+            } as const,
+            ...handlers,
+          };
+
+          const renderedItem = React.cloneElement(
+            props.onRender({
+              item,
+              active,
+            }),
+            {
+              ref: virtualRow.measureRef,
+            }
+          );
+
+          if (url) {
+            return (
+              <a
+                key={virtualRow.index}
+                href={url}
+                // eslint-disable-next-line @typescript-eslint/consistent-type-assertions
+                ref={active ? (activeRef as React.RefObject<HTMLAnchorElement>) : null}
+                {...childProps}
+              >
+                {renderedItem}
+              </a>
+            );
+          }
+
+          return (
+            <div
+              key={virtualRow.index}
+              // eslint-disable-next-line @typescript-eslint/consistent-type-assertions
+              ref={active ? (activeRef as React.RefObject<HTMLDivElement>) : null}
+              {...childProps}
+            >
+              {renderedItem}
+            </div>
+          );
+        })}
+      </div>
+    </div>
+  );
+};

public/app/features/commandPalette/ResultItem.tsx

@@ -35,8 +35,11 @@ export const ResultItem = React.forwardRef(
 
     let name = action.name;
 
+    // eslint-disable-next-line @typescript-eslint/consistent-type-assertions
+    const hasAction = Boolean(action.command?.perform || (action as ActionImpl & { url?: string }).url);
+
     // TODO: does this needs adjusting for i18n?
-    if (action.children && !action.command?.perform && !name.endsWith('...')) {
+    if (action.children.length && !hasAction && !name.endsWith('...')) {
       name += '...';
     }
 

public/app/features/commandPalette/actions/dashboardActions.ts

@@ -2,7 +2,6 @@ import debounce from 'debounce-promise';
 import { useEffect, useState } from 'react';
 
 import { locationUtil } from '@grafana/data';
-import { locationService } from '@grafana/runtime';
 import { t } from 'app/core/internationalization';
 import impressionSrv from 'app/core/services/impression_srv';
 import { getGrafanaSearcher } from 'app/features/search/service';
@@ -38,9 +37,7 @@ export async function getRecentDashboardActions(): Promise<CommandPaletteAction[
       name: `${name}`,
       section: t('command-palette.section.recent-dashboards', 'Recent dashboards'),
       priority: RECENT_DASHBOARDS_PRORITY,
-      perform: () => {
+      url: locationUtil.stripBaseFromUrl(url),
-        locationService.push(locationUtil.stripBaseFromUrl(url));
-      },
     };
   });
 
@@ -66,9 +63,7 @@ export async function getDashboardSearchResultActions(searchQuery: string): Prom
       name: `${name}`,
       section: t('command-palette.section.dashboard-search-results', 'Dashboards'),
       priority: SEARCH_RESULTS_PRORITY,
-      perform: () => {
+      url: locationUtil.stripBaseFromUrl(url),
-        locationService.push(locationUtil.stripBaseFromUrl(url));
-      },
     };
   });
 

public/app/features/commandPalette/actions/staticActions.ts

@@ -31,7 +31,7 @@ function navTreeToActions(navTree: NavModelItem[], parent?: NavModelItem): Comma
       id: idForNavItem(navItem),
       name: text, // TODO: translate
       section: section,
-      perform: url ? () => locationService.push(locationUtil.stripBaseFromUrl(url)) : undefined,
+      url: url && locationUtil.stripBaseFromUrl(url),
       parent: parent && idForNavItem(parent),
       priority: DEFAULT_PRIORITY,
     };

public/app/features/commandPalette/types.ts

@@ -9,10 +9,11 @@ export type CommandPaletteAction = RootCommandPaletteAction | ChildCommandPalett
 type RootCommandPaletteAction = Omit<Action, 'parent'> & {
   section: NotNullable<Action['section']>;
   priority: NotNullable<Action['priority']>;
+  url?: string;
 };
 
 type ChildCommandPaletteAction = Action & {
   parent: NotNullable<Action['parent']>;
-
   priority: NotNullable<Action['priority']>;
+  url?: string;
 };

yarn.lock

@@ -22260,6 +22260,7 @@ __metadata:
     react-test-renderer: 17.0.2
     react-transition-group: 4.4.5
     react-use: 17.4.0
+    react-virtual: 2.8.2
     react-virtualized-auto-sizer: 1.0.7
     react-window: 1.8.8
     react-window-infinite-loader: 1.0.8
@@ -33301,6 +33302,17 @@ __metadata:
   languageName: node
   linkType: hard
 
+"react-virtual@npm:2.8.2":
+  version: 2.8.2
+  resolution: "react-virtual@npm:2.8.2"
+  dependencies:
+    "@reach/observe-rect": ^1.1.0
+  peerDependencies:
+    react: ^16.6.3 || ^17.0.0
+  checksum: 3c95c7ea951d33d6da8d5461ea28b39dea7bd536b06ccae58ac808907761bc2425dcb469be5618c95a6f9f021f70b8019f386d21d33c64540d051f11e3f10e4a
+  languageName: node
+  linkType: hard
+
 "react-virtual@npm:^2.8.2":
   version: 2.10.4
   resolution: "react-virtual@npm:2.10.4"