diff --git a/e2e/test/scenarios/onboarding/command-palette.cy.spec.js b/e2e/test/scenarios/onboarding/command-palette.cy.spec.js index 258c10c1e1b64af1595e199dbc84321a26336ed0..480fe8704d45d1d6baf5de1e2b8dbfa1289f8eec 100644 --- a/e2e/test/scenarios/onboarding/command-palette.cy.spec.js +++ b/e2e/test/scenarios/onboarding/command-palette.cy.spec.js @@ -1,5 +1,8 @@ import { USERS } from "e2e/support/cypress_data"; -import { ORDERS_DASHBOARD_ID } from "e2e/support/cypress_sample_instance_data"; +import { + ORDERS_DASHBOARD_ID, + ORDERS_COUNT_QUESTION_ID, +} from "e2e/support/cypress_sample_instance_data"; import { restore, openCommandPalette, @@ -22,6 +25,11 @@ describe("command palette", () => { }); it("should render a searchable command palette", () => { + //Add a description for a check + cy.request("PUT", `/api/card/${ORDERS_COUNT_QUESTION_ID}`, { + description: "The best question", + }); + //Request to have an item in the recents list cy.request(`/api/dashboard/${ORDERS_DASHBOARD_ID}`); cy.visit("/"); @@ -56,10 +64,9 @@ describe("command palette", () => { cy.log("Should search entities and docs"); commandPaletteSearch().type("Orders, Count"); - cy.findByRole("option", { name: "Orders, Count" }).should( - "contain.text", - "Our analytics", - ); + cy.findByRole("option", { name: "Orders, Count" }) + .should("contain.text", "Our analytics") + .should("contain.text", "The best question"); cy.findByText('Search documentation for "Orders, Count"').should("exist"); diff --git a/frontend/src/metabase/palette/components/PaletteResultItem.tsx b/frontend/src/metabase/palette/components/PaletteResultItem.tsx index 7025dd5daec0b234683369aacc5e98754063f6fa..1097aea62a0fffcfafdec1d9781c55a56521e8bc 100644 --- a/frontend/src/metabase/palette/components/PaletteResultItem.tsx +++ b/frontend/src/metabase/palette/components/PaletteResultItem.tsx @@ -1,12 +1,12 @@ import { t } from "ttag"; import { color } from "metabase/lib/colors"; -import { Flex, Text, Icon, Box, type IconName } from "metabase/ui"; +import { Flex, Text, Icon, Box } from "metabase/ui"; -import type { PaletteAction } from "../types"; +import type { PaletteActionImpl } from "../types"; interface PaletteResultItemProps { - item: PaletteAction; + item: PaletteActionImpl; active: boolean; } @@ -35,22 +35,29 @@ export const PaletteResultItem = ({ item, active }: PaletteResultItemProps) => { c={active ? color("white") : color("text-dark")} aria-label={item.name} > - <Flex gap=".5rem" style={{ minWidth: 0 }}> - {item.icon && ( - <Icon - aria-hidden - name={(item.icon as IconName) || "click"} - color={iconColor} - style={{ - flexBasis: "16px", - }} - /> - )} + {/** Icon Container */} + {item.icon && ( + <Icon + aria-hidden + name={item.icon || "click"} + color={iconColor} + style={{ + flexBasis: "16px", + }} + /> + )} + {/**Text container */} + <Flex + direction="column" + style={{ + flexGrow: 1, + flexBasis: 0, + overflowX: "hidden", + }} + > <Box component="span" style={{ - flexGrow: 1, - flexBasis: 0, textOverflow: "ellipsis", overflow: "hidden", whiteSpace: "nowrap", @@ -80,7 +87,20 @@ export const PaletteResultItem = ({ item, active }: PaletteResultItemProps) => { >{`— ${parentName}`}</Text> )} </Box> + <Text + component="span" + color={active ? "white" : "text-light"} + fw="normal" + style={{ + textOverflow: "ellipsis", + overflow: "hidden", + whiteSpace: "nowrap", + }} + > + {item.subtitle} + </Text> </Flex> + {/** Active container */} {active && ( <Flex aria-hidden diff --git a/frontend/src/metabase/palette/components/PaletteResults.tsx b/frontend/src/metabase/palette/components/PaletteResults.tsx index 641f379324b1f868f93406420a63582b48bc22fc..1d0932ac0d293a69734dd542cedb0f73dc2e95fa 100644 --- a/frontend/src/metabase/palette/components/PaletteResults.tsx +++ b/frontend/src/metabase/palette/components/PaletteResults.tsx @@ -1,46 +1,32 @@ -import { useKBar, useMatches, KBarResults } from "kbar"; -import { useState, useMemo } from "react"; -import { useDebounce, useKeyPressEvent } from "react-use"; +import { useKBar, useMatches } from "kbar"; +import { useMemo } from "react"; +import { useKeyPressEvent } from "react-use"; import _ from "underscore"; import { color } from "metabase/lib/colors"; -import { SEARCH_DEBOUNCE_DURATION } from "metabase/lib/constants"; import { Flex, Box } from "metabase/ui"; import { useCommandPalette } from "../hooks/useCommandPalette"; -import type { PaletteAction } from "../types"; -import { processResults, findClosesestActionIndex } from "../utils"; +import type { PaletteActionImpl } from "../types"; +import { processResults, findClosestActionIndex } from "../utils"; import { PaletteResultItem } from "./PaletteResultItem"; +import { PaletteResultList } from "./PaletteResultsList"; const PAGE_SIZE = 4; export const PaletteResults = () => { // Used for finding actions within the list - const { searchQuery, query } = useKBar(state => ({ - searchQuery: state.searchQuery, - })); - const trimmedQuery = searchQuery.trim(); + const { query } = useKBar(); - // Used for finding objects across the Metabase instance - const [debouncedSearchText, setDebouncedSearchText] = useState(trimmedQuery); - - useDebounce( - () => { - setDebouncedSearchText(trimmedQuery); - }, - SEARCH_DEBOUNCE_DURATION, - [trimmedQuery], - ); - - useCommandPalette({ - query: trimmedQuery, - debouncedSearchText, - }); + useCommandPalette(); const { results } = useMatches(); - const processedResults = useMemo(() => processResults(results), [results]); + const processedResults = useMemo( + () => processResults(results as (PaletteActionImpl | string)[]), + [results], + ); useKeyPressEvent("End", () => { const lastIndex = processedResults.length - 1; @@ -53,26 +39,26 @@ export const PaletteResults = () => { useKeyPressEvent("PageDown", () => { query.setActiveIndex(i => - findClosesestActionIndex(processedResults, i, PAGE_SIZE), + findClosestActionIndex(processedResults, i, PAGE_SIZE), ); }); useKeyPressEvent("PageUp", () => { query.setActiveIndex(i => - findClosesestActionIndex(processedResults, i, -PAGE_SIZE), + findClosestActionIndex(processedResults, i, -PAGE_SIZE), ); }); return ( <Flex align="stretch" direction="column" p="0.75rem 0"> - <KBarResults + <PaletteResultList items={processedResults} // items needs to be a stable reference, otherwise the activeIndex will constantly be hijacked maxHeight={530} onRender={({ item, active, }: { - item: string | PaletteAction; + item: string | PaletteActionImpl; active: boolean; }) => { const isFirst = processedResults[0] === item; diff --git a/frontend/src/metabase/palette/components/PaletteResults.unit.spec.tsx b/frontend/src/metabase/palette/components/PaletteResults.unit.spec.tsx index d568bee35fc8d053687fc0b356d75b24c0314d19..b7e453b6a56e64a81cc343fd60c2e81ce755fae9 100644 --- a/frontend/src/metabase/palette/components/PaletteResults.unit.spec.tsx +++ b/frontend/src/metabase/palette/components/PaletteResults.unit.spec.tsx @@ -10,10 +10,9 @@ import { import { renderWithProviders, screen, - mockGetBoundingClientRect, within, waitFor, - mockOffsetHeightAndWidth, + mockScrollTo, } from "__support__/ui"; import { getAdminPaths } from "metabase/admin/app/reducers"; import { @@ -74,6 +73,7 @@ const dashboard = createMockCollectionItem({ model: "dashboard", name: "Bar Dashboard", collection: collection_1, + description: "Such Bar. Much Wow.", }); const recents_1 = createMockRecentItem({ @@ -93,8 +93,7 @@ const recents_2 = createMockRecentItem({ }), }); -mockGetBoundingClientRect(); -mockOffsetHeightAndWidth(10); // This is absurdley small, but it allows all the items to render in the "virtual list" +mockScrollTo(); const setup = ({ query }: { query?: string } = {}) => { setupDatabasesEndpoints([DATABASE]); @@ -165,6 +164,10 @@ describe("PaletteResults", () => { expect( await screen.findByRole("option", { name: "Bar Dashboard" }), ).toBeInTheDocument(); + + expect( + await screen.findByRole("option", { name: "Bar Dashboard" }), + ).toHaveTextContent("Such Bar. Much Wow."); expect( await screen.findByText('Search documentation for "Bar"'), ).toBeInTheDocument(); diff --git a/frontend/src/metabase/palette/components/PaletteResultsList.tsx b/frontend/src/metabase/palette/components/PaletteResultsList.tsx new file mode 100644 index 0000000000000000000000000000000000000000..7411e48c9dcb9f797a1357e2336724f7b670c6c8 --- /dev/null +++ b/frontend/src/metabase/palette/components/PaletteResultsList.tsx @@ -0,0 +1,186 @@ +/** + * This component was actually copied from the kbar library, but + * modified to remove virtualization of the list. This was due to virtualization + * libraries not handling dynamically sized lists where the list changes from render to + * render very well (it seemed to recompute when the list length changed, not the contents) + * + * Original can be found at https://github.com/timc1/kbar/blob/846b2c1a89f6cbff1ce947b82d04cb96a5066fbb/src/KBarResults.tsx + */ + +import { useKBar, KBAR_LISTBOX, getListboxItemId } from "kbar"; +import * as React from "react"; + +import type { PaletteActionImpl } from "../types"; + +const START_INDEX = 0; + +interface RenderParams<T = PaletteActionImpl | string> { + item: T; + active: boolean; +} + +interface PaletteResultListProps { + items: (PaletteActionImpl | string)[]; + onRender: (params: RenderParams) => React.ReactElement; + maxHeight?: number; +} + +export const PaletteResultList: React.FC<PaletteResultListProps> = props => { + const activeRef = React.useRef<HTMLDivElement>(null); + const parentRef = React.useRef<HTMLDivElement>(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 { query, search, currentRootActionId, activeIndex, options } = useKBar( + state => ({ + search: state.searchQuery, + currentRootActionId: state.currentRootActionId, + activeIndex: state.activeIndex, + }), + ); + + React.useEffect(() => { + const handler = (event: KeyboardEvent) => { + if (event.isComposing) { + return; + } + + if (event.key === "ArrowUp" || (event.ctrlKey && event.key === "p")) { + event.preventDefault(); + event.stopPropagation(); + 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(); + event.stopPropagation(); + 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(); + event.stopPropagation(); + // 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, { capture: true }); + return () => + window.removeEventListener("keydown", handler, { capture: true }); + }, [query]); + + React.useEffect(() => { + if (activeIndex > 1) { + activeRef.current?.scrollIntoView({ + behavior: "smooth", + block: "nearest", + }); + } else { + parentRef.current?.scrollTo({ top: 0, behavior: "smooth" }); + } + }, [activeIndex]); + + 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( + (item: RenderParams["item"]) => { + if (typeof item === "string") { + return; + } + if (item.command) { + item.command.perform(item); + query.toggle(); + } else { + query.setSearch(""); + query.setCurrentRootAction(item.id); + } + options.callbacks?.onSelectAction?.(item); + }, + [query, options], + ); + + return ( + <div + ref={parentRef} + style={{ + maxHeight: props.maxHeight || 400, + overflow: "auto", + }} + > + <div + role="listbox" + id={KBAR_LISTBOX} + style={{ + position: "relative", + width: "100%", + }} + > + {props.items.map((item, index) => { + const handlers = typeof item !== "string" && { + onPointerMove: () => + activeIndex !== index && query.setActiveIndex(index), + onPointerDown: () => query.setActiveIndex(index), + onClick: () => execute(item), + }; + const active = index === activeIndex; + + return ( + <div + ref={active ? activeRef : null} + id={getListboxItemId(index)} + role="option" + aria-selected={active} + key={typeof item === "string" ? item : item.id} + {...handlers} + > + {React.cloneElement( + props.onRender({ + item, + active, + }), + )} + </div> + ); + })} + </div> + </div> + ); +}; diff --git a/frontend/src/metabase/palette/hooks/useCommandPalette.tsx b/frontend/src/metabase/palette/hooks/useCommandPalette.tsx index b5c6b3c3a695fa61a9a26e7f966da5c997d931da..51ffde49d7bb79bc8c43de066de059a8f0920521 100644 --- a/frontend/src/metabase/palette/hooks/useCommandPalette.tsx +++ b/frontend/src/metabase/palette/hooks/useCommandPalette.tsx @@ -1,14 +1,15 @@ -import { useRegisterActions } from "kbar"; -import { useMemo } from "react"; +import { useRegisterActions, useKBar } from "kbar"; +import { useMemo, useState } from "react"; import { push } from "react-router-redux"; +import { useDebounce } from "react-use"; import { t } from "ttag"; import { getAdminPaths } from "metabase/admin/app/selectors"; import { getSectionsWithPlugins } from "metabase/admin/settings/selectors"; -import { useListRecentItemsQuery, skipToken } from "metabase/api"; -import { useSearchListQuery } from "metabase/common/hooks"; +import { useListRecentItemsQuery, useSearchQuery } from "metabase/api"; import { ROOT_COLLECTION } from "metabase/entities/collections"; import Search from "metabase/entities/search"; +import { SEARCH_DEBOUNCE_DURATION } from "metabase/lib/constants"; import { getIcon } from "metabase/lib/icon"; import { getName } from "metabase/lib/name"; import { useDispatch, useSelector } from "metabase/lib/redux"; @@ -20,42 +21,52 @@ import { getSettings, } from "metabase/selectors/settings"; import { getShowMetabaseLinks } from "metabase/selectors/whitelabel"; -import type { SearchResult } from "metabase-types/api"; import type { PaletteAction } from "../types"; -export type PalettePageId = "root" | "admin_settings"; - -export const useCommandPalette = ({ - query, - debouncedSearchText, -}: { - query: string; - debouncedSearchText: string; -}) => { +export const useCommandPalette = () => { const dispatch = useDispatch(); const docsUrl = useSelector(state => getDocsUrl(state, {})); const showMetabaseLinks = useSelector(getShowMetabaseLinks); - const hasQuery = query.length > 0; + // Used for finding actions within the list + const { searchQuery } = useKBar(state => ({ + searchQuery: state.searchQuery, + })); + const trimmedQuery = searchQuery.trim(); + + // Used for finding objects across the Metabase instance + const [debouncedSearchText, setDebouncedSearchText] = useState(trimmedQuery); + + useDebounce( + () => { + setDebouncedSearchText(trimmedQuery); + }, + SEARCH_DEBOUNCE_DURATION, + [trimmedQuery], + ); + + const hasQuery = searchQuery.length > 0; const { - data: searchResults, + currentData: searchResults, + isFetching: isSearchLoading, error: searchError, - isLoading: isSearchLoading, - } = useSearchListQuery<SearchResult>({ - enabled: !!debouncedSearchText, - query: { q: debouncedSearchText, limit: 20 }, - reload: true, - }); - - const { data: recentItems } = useListRecentItemsQuery( - debouncedSearchText ? skipToken : undefined, + } = useSearchQuery( + { + q: debouncedSearchText, + limit: 20, + }, { + skip: !debouncedSearchText, refetchOnMountOrArgChange: true, }, ); + const { data: recentItems } = useListRecentItemsQuery(undefined, { + refetchOnMountOrArgChange: true, + }); + const adminPaths = useSelector(getAdminPaths); const settingValues = useSelector(getSettings); const settingsSections = useMemo<Record<string, any>>( @@ -67,15 +78,15 @@ export const useCommandPalette = ({ const ret: PaletteAction[] = [ { id: "search_docs", - name: query - ? `Search documentation for "${query}"` + name: debouncedSearchText + ? `Search documentation for "${debouncedSearchText}"` : t`View documentation`, section: "docs", - keywords: query, // Always match the query string + keywords: debouncedSearchText, // Always match the debouncedSearchText string icon: "document", perform: () => { - if (query) { - window.open(getDocsSearchUrl({ query })); + if (debouncedSearchText) { + window.open(getDocsSearchUrl({ debouncedSearchText })); } else { window.open(docsUrl); } @@ -83,7 +94,7 @@ export const useCommandPalette = ({ }, ]; return ret; - }, [query, docsUrl]); + }, [debouncedSearchText, docsUrl]); const showDocsAction = showMetabaseLinks && hasQuery; @@ -98,7 +109,7 @@ export const useCommandPalette = ({ { id: "search-is-loading", name: "Loading...", - keywords: query, + keywords: searchQuery, section: "search", }, ]; @@ -111,15 +122,16 @@ export const useCommandPalette = ({ }, ]; } else if (debouncedSearchText) { - if (searchResults?.length) { - return searchResults.map(result => { + if (searchResults?.data?.length) { + return searchResults.data.map(result => { const wrappedResult = Search.wrapEntity(result, dispatch); return { - id: `search-result-${result.id}`, + id: `search-result-${result.model}-${result.id}`, name: result.name, icon: wrappedResult.getIcon().name, section: "search", keywords: debouncedSearchText, + subtitle: result.description || "", perform: () => { dispatch(closeModal()); dispatch(push(wrappedResult.getUrl())); @@ -145,8 +157,8 @@ export const useCommandPalette = ({ return []; }, [ dispatch, - query, debouncedSearchText, + searchQuery, isSearchLoading, searchError, searchResults, diff --git a/frontend/src/metabase/palette/types.ts b/frontend/src/metabase/palette/types.ts index 2f94b75ff6ebb75185207742f0d19b58186f1ffb..3d21db28a4ac3283672a158b77021e351884d60b 100644 --- a/frontend/src/metabase/palette/types.ts +++ b/frontend/src/metabase/palette/types.ts @@ -1,9 +1,23 @@ -import type { Action } from "kbar"; +import type { Action, ActionImpl } from "kbar"; -export interface PaletteAction extends Action { +import type { IconName } from "metabase/ui"; + +interface PaletteActionExtras { extra?: { parentCollection?: string | null; isVerified?: boolean; database?: string | null; }; } + +export type PaletteAction = Action & + PaletteActionExtras & { + subtitle?: Action["subtitle"]; + icon?: IconName; + }; + +export type PaletteActionImpl = ActionImpl & + PaletteActionExtras & { + subtitle?: Action["subtitle"]; + icon?: IconName; + }; diff --git a/frontend/src/metabase/palette/utils.ts b/frontend/src/metabase/palette/utils.ts index 2325297ccbd3059e442fd5111b0c7ce5d64020fe..d8587000abd7830356ab3c1f240f645178a048d6 100644 --- a/frontend/src/metabase/palette/utils.ts +++ b/frontend/src/metabase/palette/utils.ts @@ -1,12 +1,13 @@ -import type { ActionImpl } from "kbar"; import { t } from "ttag"; import _ from "underscore"; +import type { PaletteActionImpl } from "./types"; + export const processResults = ( - results: (string | ActionImpl)[], -): (string | ActionImpl)[] => { + results: (string | PaletteActionImpl)[], +): (string | PaletteActionImpl)[] => { const groupedResults = _.groupBy( - results.filter((r): r is ActionImpl => !(typeof r === "string")), + results.filter((r): r is PaletteActionImpl => !(typeof r === "string")), "section", ); @@ -19,7 +20,10 @@ export const processResults = ( return [...recent, ...actions.slice(0, 6), ...admin, ...search, ...docs]; }; -export const processSection = (sectionName: string, items?: ActionImpl[]) => { +export const processSection = ( + sectionName: string, + items?: PaletteActionImpl[], +) => { if (items && items.length > 0) { return [sectionName, ...items]; } else { @@ -27,20 +31,20 @@ export const processSection = (sectionName: string, items?: ActionImpl[]) => { } }; -export const findClosesestActionIndex = ( - actions: (string | ActionImpl)[], +export const findClosestActionIndex = ( + actions: (string | PaletteActionImpl)[], index: number, diff: number, ): number => { if (index + diff < 0) { - return findClosesestActionIndex(actions, -1, 1); + return findClosestActionIndex(actions, -1, 1); } else if (index + diff > actions.length - 1) { - return findClosesestActionIndex(actions, actions.length, -1); + return findClosestActionIndex(actions, actions.length, -1); } else if (typeof actions[index + diff] === "string") { if (diff < 0) { - return findClosesestActionIndex(actions, index, diff - 1); + return findClosestActionIndex(actions, index, diff - 1); } else { - return findClosesestActionIndex(actions, index, diff + 1); + return findClosestActionIndex(actions, index, diff + 1); } } diff --git a/frontend/src/metabase/palette/utils.unit.spec.ts b/frontend/src/metabase/palette/utils.unit.spec.ts index 14ccd5a4c64d22564a1f04c0925ceab02fe917df..8da14011c09ba7935e7837f01e7a7afea37649c9 100644 --- a/frontend/src/metabase/palette/utils.unit.spec.ts +++ b/frontend/src/metabase/palette/utils.unit.spec.ts @@ -1,5 +1,4 @@ -import type { ActionImpl } from "kbar"; - +import type { PaletteActionImpl } from "./types"; import { processResults, processSection } from "./utils"; interface mockAction { @@ -10,7 +9,7 @@ interface mockAction { const createMockAction = ({ name, section = "basic", -}: mockAction): ActionImpl => ({ name, section } as ActionImpl); +}: mockAction): PaletteActionImpl => ({ name, section } as PaletteActionImpl); describe("command palette utils", () => { describe("processSection", () => { @@ -25,7 +24,7 @@ describe("command palette utils", () => { expect(result[0]).toBe("Basic"); }); it("should return an empty list if there are no items", () => { - const items: ActionImpl[] = []; + const items: PaletteActionImpl[] = []; const result = processSection("Basic", items); expect(result).toHaveLength(0); }); diff --git a/frontend/test/__support__/ui.tsx b/frontend/test/__support__/ui.tsx index b4b964b6694fff2d28060c50bab8da8ecf914050..129f2fbc6acef3f1467b99ac68125e36af212db7 100644 --- a/frontend/test/__support__/ui.tsx +++ b/frontend/test/__support__/ui.tsx @@ -319,6 +319,13 @@ export const mockScrollBy = () => { window.Element.prototype.scrollBy = jest.fn(); }; +/** + * jsdom doesn't have scrollBy, so we need to mock it + */ +export const mockScrollTo = () => { + window.Element.prototype.scrollTo = jest.fn(); +}; + /** * jsdom doesn't have DataTransfer */