From d284cceb676ea28905508803cc6bd4294ca2cad4 Mon Sep 17 00:00:00 2001 From: Andrej Ocenas Date: Thu, 12 Jun 2025 11:53:04 +0200 Subject: [PATCH] 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 --- .../commandPalette/CommandPalette.tsx | 14 ++-- .../actions/dashboardActions.test.ts | 42 +++++++++++- .../actions/dashboardActions.ts | 8 +-- .../commandPalette/actions/useActions.tsx | 66 +++++++++++-------- 4 files changed, 91 insertions(+), 39 deletions(-) diff --git a/public/app/features/commandPalette/CommandPalette.tsx b/public/app/features/commandPalette/CommandPalette.tsx index 26c0eda306b..ac4d2f07acc 100644 --- a/public/app/features/commandPalette/CommandPalette.tsx +++ b/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(null); const { overlayProps } = useOverlay( - { isOpen: showing, onClose: () => query.setVisualState(VisualState.animatingOut) }, + { isOpen: true, onClose: () => query.setVisualState(VisualState.animatingOut) }, ref ); diff --git a/public/app/features/commandPalette/actions/dashboardActions.test.ts b/public/app/features/commandPalette/actions/dashboardActions.test.ts index 5e6e390e32f..15778f6f6c2 100644 --- a/public/app/features/commandPalette/actions/dashboardActions.test.ts +++ b/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', + }, + ]); + }); + }); + }); }); diff --git a/public/app/features/commandPalette/actions/dashboardActions.ts b/public/app/features/commandPalette/actions/dashboardActions.ts index 8a7ff8cfb1b..f2d3aea83fa 100644 --- a/public/app/features/commandPalette/actions/dashboardActions.ts +++ b/public/app/features/commandPalette/actions/dashboardActions.ts @@ -81,10 +81,8 @@ export async function getSearchResultActions(searchQuery: string): Promise([]); const [isFetchingSearchResults, setIsFetchingSearchResults] = useState(false); const lastSearchTimestamp = useRef(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, diff --git a/public/app/features/commandPalette/actions/useActions.tsx b/public/app/features/commandPalette/actions/useActions.tsx index 47c4db309dd..1a88caa451c 100644 --- a/public/app/features/commandPalette/actions/useActions.tsx +++ b/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([]); 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