Command palette: Don't show dashboard search result from different parent (#106536)

* Don't show result from other parents

* Use global search hook for scopes even inside scopes category

* Remove console.log

* Don't show non leaf scopes in global search

public/app/features/commandPalette/CommandPalette.tsx

@@ -42,24 +42,28 @@ function CommandPaletteContents() {
   const lateralSpace = getCommandPalettePosition();
   const styles = useStyles2(getSearchStyles, lateralSpace);
 
-  const { query, showing, searchQuery, currentRootActionId } = useKBar((state) => ({
+  const { query, searchQuery, currentRootActionId } = useKBar((state) => ({
     showing: state.visualState === VisualState.showing,
     searchQuery: state.searchQuery,
     currentRootActionId: state.currentRootActionId,
   }));
 
-  useRegisterRecentDashboardsActions(searchQuery);
+  useRegisterRecentDashboardsActions();
   useRegisterRecentScopesActions();
 
   const queryToggle = useCallback(() => query.toggle(), [query]);
   const { scopesRow } = useRegisterScopesActions(searchQuery, queryToggle, currentRootActionId);
 
-  // Dashboards and folders
-  const { searchResults, isFetchingSearchResults } = useSearchResults(searchQuery, showing);
+  // This searches dashboards and folders it shows only if we are not in some specific category (and there is no
+  // dashboards category right now, so if any category is selected, we don't show these).
+  // Normally we register actions with kbar, and it knows not to show actions which are under a different parent than is
+  // the currentRootActionId. Because these search results are manually added to the list later, they would show every
+  // time.
+  const { searchResults, isFetchingSearchResults } = useSearchResults({ searchQuery, show: !currentRootActionId });
 
   const ref = useRef<HTMLDivElement>(null);
   const { overlayProps } = useOverlay(
-    { isOpen: showing, onClose: () => query.setVisualState(VisualState.animatingOut) },
+    { isOpen: true, onClose: () => query.setVisualState(VisualState.animatingOut) },
     ref
   );
 

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

@@ -1,3 +1,5 @@
+import { renderHook, waitFor } from '@testing-library/react';
+
 import { DataFrame, DataFrameView, FieldType } from '@grafana/data';
 import { config } from '@grafana/runtime';
 import { ContextSrv, contextSrv } from 'app/core/services/context_srv';
@@ -5,7 +7,7 @@ import impressionSrv from 'app/core/services/impression_srv';
 import { getGrafanaSearcher } from 'app/features/search/service/searcher';
 import { DashboardQueryResult, QueryResponse } from 'app/features/search/service/types';
 
-import { getRecentDashboardActions, getSearchResultActions } from './dashboardActions';
+import { getRecentDashboardActions, getSearchResultActions, useSearchResults } from './dashboardActions';
 
 describe('dashboardActions', () => {
   let grafanaSearcherSpy: jest.SpyInstance;
@@ -166,4 +168,42 @@ describe('dashboardActions', () => {
       });
     });
   });
+
+  describe('useSearchResults', () => {
+    it('returns an empty array if the search query is empty', async () => {
+      const { result } = renderHook(() => {
+        return useSearchResults({ searchQuery: '', show: true });
+      });
+      expect(result.current.searchResults).toEqual([]);
+      expect(result.current.isFetchingSearchResults).toEqual(false);
+    });
+
+    it('returns an empty array if show is false', async () => {
+      const { result } = renderHook(() => {
+        return useSearchResults({ searchQuery: 'something', show: false });
+      });
+      expect(result.current.searchResults).toEqual([]);
+      expect(result.current.isFetchingSearchResults).toBe(false);
+    });
+
+    it('returns dashboard actions', async () => {
+      mockContextSrv.user.isSignedIn = true;
+      const { result } = renderHook(() => {
+        return useSearchResults({ searchQuery: 'mySearchQuery', show: true });
+      });
+      expect(result.current.isFetchingSearchResults).toBe(true);
+      await waitFor(() => {
+        expect(result.current.searchResults).toEqual([
+          {
+            id: 'go/dashboard/my-dashboard-1',
+            name: 'My dashboard 1',
+            priority: 1,
+            section: 'Dashboards',
+            subtitle: 'My folder 1',
+            url: '/my-dashboard-1',
+          },
+        ]);
+      });
+    });
+  });
 });

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

@@ -81,10 +81,8 @@ export async function getSearchResultActions(searchQuery: string): Promise<Comma
 
 /**
  * Implements actual search logic for dashboards and folders.
- * @param searchQuery
- * @param isShowing
  */
-export function useSearchResults(searchQuery: string, isShowing: boolean) {
+export function useSearchResults({ searchQuery, show }: { searchQuery: string; show: boolean }) {
   const [searchResults, setSearchResults] = useState<CommandPaletteAction[]>([]);
   const [isFetchingSearchResults, setIsFetchingSearchResults] = useState(false);
   const lastSearchTimestamp = useRef<number>(0);
@@ -92,7 +90,7 @@ export function useSearchResults(searchQuery: string, isShowing: boolean) {
   // Hit dashboards API
   useEffect(() => {
     const timestamp = Date.now();
-    if (isShowing && searchQuery.length > 0) {
+    if (show && searchQuery.length > 0) {
       setIsFetchingSearchResults(true);
       debouncedSearch(searchQuery).then((resultActions) => {
         // Only keep the results if it's was issued after the most recently resolved search.
@@ -110,7 +108,7 @@ export function useSearchResults(searchQuery: string, isShowing: boolean) {
       setIsFetchingSearchResults(false);
       lastSearchTimestamp.current = timestamp;
     }
-  }, [isShowing, searchQuery]);
+  }, [show, searchQuery]);
 
   return {
     searchResults,

public/app/features/commandPalette/actions/useActions.tsx

@@ -34,17 +34,15 @@ export function useRegisterStaticActions() {
   useRegisterActions(navTreeActions, [navTreeActions]);
 }
 
-export function useRegisterRecentDashboardsActions(searchQuery: string) {
+export function useRegisterRecentDashboardsActions() {
   const [recentDashboardActions, setRecentDashboardActions] = useState<CommandPaletteAction[]>([]);
   useEffect(() => {
-    if (!searchQuery) {
-      getRecentDashboardActions()
-        .then((recentDashboardActions) => setRecentDashboardActions(recentDashboardActions))
-        .catch((err) => {
-          console.error('Error loading recent dashboard actions', err);
-        });
-    }
-  }, [searchQuery]);
+    getRecentDashboardActions()
+      .then((recentDashboardActions) => setRecentDashboardActions(recentDashboardActions))
+      .catch((err) => {
+        console.error('Error loading recent dashboard actions', err);
+      });
+  }, []);
 
   useRegisterActions(recentDashboardActions, [recentDashboardActions]);
 }
@@ -77,7 +75,7 @@ export function useRegisterScopesActions(
   const { updateNode, selectScope, deselectScope, apply, resetSelection, searchAllNodes } =
     services.scopesSelectorService;
 
-  // Initialize the scopes first time this runs and reset the scopes that were selected on unmount.
+  // Initialize the scopes the first time this runs and reset the scopes that were selected on unmount.
   useEffect(() => {
     updateNode('', true, '');
     resetSelection();
@@ -90,9 +88,11 @@ export function useRegisterScopesActions(
 
   // Load the next level of scopes when the parentId changes.
   useEffect(() => {
-    if (parentId) {
-      updateNode(parentId === 'scopes' ? '' : last(parentId.split('/'))!, true, searchQuery);
+    // This is the case where we do global search instead of loading the nodes in a tree.
+    if (!parentId || (parentId === 'scopes' && searchQuery)) {
+      return;
     }
+    updateNode(parentId === 'scopes' ? '' : last(parentId.split('/'))!, true, searchQuery);
   }, [updateNode, searchQuery, parentId]);
 
   const selectorServiceState: ScopesSelectorServiceState | undefined = useObservable(
@@ -103,11 +103,16 @@ export function useRegisterScopesActions(
   const { nodes, scopes, tree, selectedScopes, appliedScopes } = selectorServiceState;
 
   const nodesActions = useMemo(() => {
-    // If we have nodes from global search, we show those in a flat list.
-    return globalNodes
-      ? Object.values(globalNodes).map((node) => mapScopeNodeToAction(node, selectScope))
-      : mapScopesNodesTreeToActions(nodes, tree!, selectedScopes, selectScope);
-  }, [globalNodes, nodes, tree, selectedScopes, selectScope]);
+    if (globalNodes) {
+      // If we have nodes from global search, we show those in a flat list.
+      const actions = [getScopesParentAction()];
+      for (const node of Object.values(globalNodes)) {
+        actions.push(mapScopeNodeToAction(node, selectScope, parentId || undefined));
+      }
+      return actions;
+    }
+    return mapScopesNodesTreeToActions(nodes, tree!, selectedScopes, selectScope);
+  }, [globalNodes, nodes, tree, selectedScopes, selectScope, parentId]);
 
   useRegisterActions(nodesActions, [nodesActions]);
 
@@ -164,12 +169,15 @@ function useGlobalScopesSearch(
 
   // Load next level of scopes when the parentId changes.
   useEffect(() => {
-    if (!parentId && searchQuery && config.featureToggles.scopeSearchAllLevels) {
+    if ((!parentId || parentId === 'scopes') && searchQuery && config.featureToggles.scopeSearchAllLevels) {
       // We only search globally if there is no parentId
       searchQueryRef.current = searchQuery;
       searchAllNodes(searchQuery, 10).then((nodes) => {
         if (searchQueryRef.current === searchQuery) {
-          const nodesMap = fromPairs(nodes.map((n) => [n.metadata.name, n]));
+          // Only show leaf nodes because otherwise there are issues with navigating to a category without knowing
+          // where in the tree it is.
+          const leafNodes = nodes.filter((node) => node.spec.nodeType === 'leaf');
+          const nodesMap = fromPairs(leafNodes.map((n) => [n.metadata.name, n]));
           setNodes(nodesMap);
         }
       });
@@ -182,21 +190,23 @@ function useGlobalScopesSearch(
   return nodes;
 }
 
+function getScopesParentAction(): CommandPaletteAction {
+  return {
+    id: 'scopes',
+    section: t('command-palette.action.scopes', 'Scopes'),
+    name: t('command-palette.action.scopes', 'Scopes'),
+    keywords: 'scopes filters',
+    priority: SCOPES_PRIORITY,
+  };
+}
+
 function mapScopesNodesTreeToActions(
   nodes: NodesMap,
   tree: TreeNode,
   selectedScopes: SelectedScope[],
   selectScope: (id: string) => void
 ): CommandPaletteAction[] {
-  const actions: CommandPaletteAction[] = [
-    {
-      id: 'scopes',
-      section: t('command-palette.action.scopes', 'Scopes'),
-      name: t('command-palette.action.scopes', 'Scopes'),
-      keywords: 'scopes filters',
-      priority: SCOPES_PRIORITY,
-    },
-  ];
+  const actions: CommandPaletteAction[] = [getScopesParentAction()];
 
   const traverse = (tree: TreeNode, parentId: string | undefined) => {
     // TODO: not sure how and why a node.nodes can be undefined