Refactor command palette to distinguish between live and debounced search queries (#73921)
* Surface command palette static actions without waiting for debounce (GDGT-2409) * Add test that static actions surface before search debounce * Update palette tests for new processResults signature and PaletteResultList prop * Rename appliedRemoteSearchTerm to debouncedSearchTerm, refactor debounce test Address PR review feedback: - Rename `appliedRemoteSearchTerm` to `debouncedSearchTerm` for clarity - Replace the 200ms timeout in the static-actions test with an assertion on `/api/search` call count, decoupling the test from the debounce duration * Fix command palette getting stuck on whitespace-suffixed queries kbar feeds the raw search query to Fuse.js, whose Bitap-based scoring penalizes the per-character length of the query. A query with trailing whitespace (e.g. "AD. ") tanks the score for every action below the 0.3 threshold, leaving the panel stuck on its loading skeleton. Set the `keywords` field on every search-derived action to kbar's raw `searchQuery` so Fuse can match it verbatim. Combined per-key scores in Fuse are multiplicative — a single perfect-match key drives the total score to ~EPSILON regardless of how poorly other keys match — so a single keyword identical to the input is enough to surface the action. * Fix matching on leading spaces
This commit is contained in:
@@ -21,7 +21,7 @@ const AppPaletteActions = () => {
|
||||
const { searchQuery } = useKBar((state) => ({
|
||||
searchQuery: state.searchQuery,
|
||||
}));
|
||||
const hasQuery = searchQuery.length > 0;
|
||||
const hasQuery = searchQuery.trim().length > 0;
|
||||
|
||||
const adminActions = useMemo<PaletteAction[]>(() => {
|
||||
const adminSubpaths = isAdmin
|
||||
|
||||
@@ -56,10 +56,15 @@ export const PaletteContainer = withRouter(
|
||||
disabled: boolean;
|
||||
locationQuery: Query;
|
||||
}) => {
|
||||
const { query } = useKBar((state) => ({ actions: state.actions }));
|
||||
const { query } = useKBar();
|
||||
const ref = useRef(null);
|
||||
|
||||
const { searchRequestId, searchResults, searchTerm } = useCommandPalette({
|
||||
const {
|
||||
searchRequestId,
|
||||
searchResults,
|
||||
liveSearchTerm,
|
||||
debouncedSearchTerm,
|
||||
} = useCommandPalette({
|
||||
locationQuery,
|
||||
disabled,
|
||||
});
|
||||
@@ -99,7 +104,8 @@ export const PaletteContainer = withRouter(
|
||||
locationQuery={locationQuery}
|
||||
searchRequestId={searchRequestId}
|
||||
searchResults={searchResults}
|
||||
searchTerm={searchTerm}
|
||||
liveSearchTerm={liveSearchTerm}
|
||||
debouncedSearchTerm={debouncedSearchTerm}
|
||||
/>
|
||||
</Stack>
|
||||
</Card>
|
||||
|
||||
@@ -53,6 +53,7 @@ const setupInList = ({ item }: { item: Partial<PaletteActionImpl> }) => {
|
||||
items={items.map((item) => mockPaletteActionImpl(item))}
|
||||
maxHeight={580}
|
||||
minHeight={220}
|
||||
liveSearchTerm=""
|
||||
renderItem={({
|
||||
item,
|
||||
active,
|
||||
|
||||
@@ -33,12 +33,12 @@ const PAGE_SIZE = 4;
|
||||
const FullSearchCTA = ({
|
||||
locationQuery,
|
||||
searchResults,
|
||||
searchTerm,
|
||||
debouncedSearchTerm,
|
||||
onClick,
|
||||
}: {
|
||||
locationQuery: Query;
|
||||
searchResults: SearchResponse;
|
||||
searchTerm: string;
|
||||
debouncedSearchTerm: string;
|
||||
onClick: () => void;
|
||||
}) => {
|
||||
const showOtherUsersCollections = useShowOtherUsersCollections();
|
||||
@@ -59,7 +59,7 @@ const FullSearchCTA = ({
|
||||
pathname: "search",
|
||||
query: {
|
||||
...locationQuery,
|
||||
q: searchTerm,
|
||||
q: debouncedSearchTerm,
|
||||
},
|
||||
}}
|
||||
className={S.viewAndFilterResults}
|
||||
@@ -81,14 +81,16 @@ type Props = Omit<StackProps, "children"> & {
|
||||
locationQuery: Query;
|
||||
searchRequestId?: string;
|
||||
searchResults?: SearchResponse;
|
||||
searchTerm: string;
|
||||
liveSearchTerm: string;
|
||||
debouncedSearchTerm: string;
|
||||
};
|
||||
|
||||
export const PaletteResults = ({
|
||||
locationQuery,
|
||||
searchRequestId,
|
||||
searchResults,
|
||||
searchTerm,
|
||||
liveSearchTerm,
|
||||
debouncedSearchTerm,
|
||||
...props
|
||||
}: Props) => {
|
||||
// Used for finding actions within the list
|
||||
@@ -97,8 +99,12 @@ export const PaletteResults = ({
|
||||
const { results } = useMatches();
|
||||
|
||||
const processedResults = useMemo(
|
||||
() => processResults(results as (PaletteActionImpl | string)[], searchTerm),
|
||||
[results, searchTerm],
|
||||
() =>
|
||||
processResults(
|
||||
results as (PaletteActionImpl | string)[],
|
||||
liveSearchTerm.length !== 0,
|
||||
),
|
||||
[results, liveSearchTerm],
|
||||
);
|
||||
|
||||
useEffect(() => {
|
||||
@@ -129,11 +135,11 @@ export const PaletteResults = ({
|
||||
);
|
||||
});
|
||||
|
||||
if (processedResults.length === 0 && searchTerm.length === 0) {
|
||||
if (processedResults.length === 0 && liveSearchTerm.length === 0) {
|
||||
return <PaletteEmptyState />;
|
||||
}
|
||||
|
||||
if (processedResults.length === 0 && searchTerm.length > 0) {
|
||||
if (processedResults.length === 0 && liveSearchTerm.length > 0) {
|
||||
return <PaletteResultsSkeleton />;
|
||||
}
|
||||
|
||||
@@ -142,7 +148,8 @@ export const PaletteResults = ({
|
||||
<PaletteResultList
|
||||
items={processedResults} // items needs to be a stable reference, otherwise the activeIndex will constantly be hijacked
|
||||
maxHeight={530}
|
||||
minHeight={searchTerm.length === 0 ? 280 : 0}
|
||||
minHeight={liveSearchTerm.length === 0 ? 280 : 0}
|
||||
liveSearchTerm={liveSearchTerm}
|
||||
renderItem={({ item, active }) => {
|
||||
const isFirst = processedResults[0] === item;
|
||||
|
||||
@@ -164,7 +171,7 @@ export const PaletteResults = ({
|
||||
<FullSearchCTA
|
||||
locationQuery={locationQuery}
|
||||
searchResults={searchResults}
|
||||
searchTerm={searchTerm}
|
||||
debouncedSearchTerm={debouncedSearchTerm}
|
||||
onClick={() => {
|
||||
query.setVisualState(VisualState.hidden);
|
||||
|
||||
@@ -176,7 +183,7 @@ export const PaletteResults = ({
|
||||
requestId: searchRequestId,
|
||||
entityModel: null,
|
||||
entityId: null,
|
||||
searchTerm,
|
||||
searchTerm: debouncedSearchTerm,
|
||||
});
|
||||
}}
|
||||
/>
|
||||
|
||||
@@ -31,9 +31,11 @@ interface PaletteResultListProps {
|
||||
renderItem: (params: RenderParams) => React.ReactElement;
|
||||
maxHeight: number;
|
||||
minHeight: number;
|
||||
liveSearchTerm: string;
|
||||
}
|
||||
|
||||
export const PaletteResultList = (props: PaletteResultListProps) => {
|
||||
const { liveSearchTerm } = props;
|
||||
const activeRef = useRef<HTMLDivElement>(null);
|
||||
const parentRef = useRef<HTMLDivElement>(null);
|
||||
|
||||
@@ -44,9 +46,8 @@ export const PaletteResultList = (props: PaletteResultListProps) => {
|
||||
|
||||
const hasUserInteractedRef = useRef(false);
|
||||
|
||||
const { query, search, currentRootActionId, activeIndex, options } = useKBar(
|
||||
const { query, currentRootActionId, activeIndex, options } = useKBar(
|
||||
(state) => ({
|
||||
search: state.searchQuery,
|
||||
currentRootActionId: state.currentRootActionId,
|
||||
activeIndex: state.activeIndex,
|
||||
}),
|
||||
@@ -120,7 +121,7 @@ export const PaletteResultList = (props: PaletteResultListProps) => {
|
||||
// Reset interaction tracking when search changes
|
||||
useEffect(() => {
|
||||
hasUserInteractedRef.current = false;
|
||||
}, [search]);
|
||||
}, [liveSearchTerm]);
|
||||
|
||||
useEffect(() => {
|
||||
// Only auto-set the active index if the user hasn't interacted yet.
|
||||
@@ -133,7 +134,7 @@ export const PaletteResultList = (props: PaletteResultListProps) => {
|
||||
: START_INDEX,
|
||||
);
|
||||
}
|
||||
}, [search, currentRootActionId, props.items, query]);
|
||||
}, [liveSearchTerm, currentRootActionId, props.items, query]);
|
||||
|
||||
const execute = useCallback(
|
||||
(item: RenderParams["item"], e?: MouseEvent) => {
|
||||
|
||||
@@ -29,7 +29,17 @@ describe("PaletteResults", () => {
|
||||
expect(await screen.findByText("New SQL query")).toBeInTheDocument();
|
||||
expect(await screen.findByText("New dashboard")).toBeInTheDocument();
|
||||
|
||||
expect(screen.getByText("Results")).toBeInTheDocument();
|
||||
expect(await screen.findByText("Results")).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("should surface static actions before the remote search debounce fires", async () => {
|
||||
setup({ query: "new" });
|
||||
|
||||
expect(await screen.findByText("New question")).toBeInTheDocument();
|
||||
|
||||
// useCommandPaletteBasicActions makes one baseline /api/search call; any
|
||||
// additional call means the debounced remote search has already run.
|
||||
expect(fetchMock.callHistory.calls("path:/api/search").length).toBe(1);
|
||||
});
|
||||
|
||||
//For some reason, New Question isn't showing up without searching. My guess is virtualization weirdness
|
||||
|
||||
@@ -34,7 +34,12 @@ import { PaletteResults } from "../../PaletteResults";
|
||||
const TestComponent = withRouter(
|
||||
({ q, ...props }: WithRouterProps & { q?: string; isLoggedIn: boolean }) => {
|
||||
useCommandPaletteBasicActions(props);
|
||||
const { searchRequestId, searchResults, searchTerm } = useCommandPalette({
|
||||
const {
|
||||
searchRequestId,
|
||||
searchResults,
|
||||
liveSearchTerm,
|
||||
debouncedSearchTerm,
|
||||
} = useCommandPalette({
|
||||
disabled: false,
|
||||
locationQuery: props.location.query,
|
||||
});
|
||||
@@ -53,7 +58,8 @@ const TestComponent = withRouter(
|
||||
locationQuery={props.location.query}
|
||||
searchRequestId={searchRequestId}
|
||||
searchResults={searchResults}
|
||||
searchTerm={searchTerm}
|
||||
liveSearchTerm={liveSearchTerm}
|
||||
debouncedSearchTerm={debouncedSearchTerm}
|
||||
/>
|
||||
);
|
||||
},
|
||||
|
||||
@@ -113,7 +113,8 @@ export const useCommandPalette = ({
|
||||
? t`Search documentation for "${debouncedSearchText}"`
|
||||
: t`View documentation`,
|
||||
section: "docs",
|
||||
keywords: debouncedSearchText, // Always match the debouncedSearchText string
|
||||
// Include search query in keywords so kbar always shows it
|
||||
keywords: trimmedQuery,
|
||||
icon: "document",
|
||||
extra: {
|
||||
href: link,
|
||||
@@ -121,7 +122,7 @@ export const useCommandPalette = ({
|
||||
},
|
||||
];
|
||||
return ret;
|
||||
}, [debouncedSearchText, docsUrl]);
|
||||
}, [debouncedSearchText, docsUrl, trimmedQuery]);
|
||||
|
||||
const showDocsAction = showMetabaseLinks && hasQuery && !disabled;
|
||||
|
||||
@@ -148,7 +149,7 @@ export const useCommandPalette = ({
|
||||
id: `search-without-typeahead`,
|
||||
name: t`View search results for "${debouncedSearchText}"`,
|
||||
section: "search",
|
||||
keywords: debouncedSearchText,
|
||||
keywords: trimmedQuery,
|
||||
icon: "link" as const,
|
||||
priority: Priority.HIGH,
|
||||
extra: {
|
||||
@@ -161,7 +162,7 @@ export const useCommandPalette = ({
|
||||
{
|
||||
id: "search-is-loading",
|
||||
name: t`Loading...`,
|
||||
keywords: searchQuery,
|
||||
keywords: trimmedQuery,
|
||||
section: "search",
|
||||
disabled: true,
|
||||
},
|
||||
@@ -186,7 +187,7 @@ export const useCommandPalette = ({
|
||||
icon: icon.name,
|
||||
iconUrl: icon.iconUrl,
|
||||
section: "search",
|
||||
keywords: debouncedSearchText,
|
||||
keywords: trimmedQuery,
|
||||
priority: Priority.NORMAL - index,
|
||||
perform: () => {
|
||||
trackSearchClick({
|
||||
@@ -213,7 +214,7 @@ export const useCommandPalette = ({
|
||||
{
|
||||
id: "no-search-results",
|
||||
name: t`No results for “${debouncedSearchText}”`,
|
||||
keywords: debouncedSearchText,
|
||||
keywords: trimmedQuery,
|
||||
section: "search",
|
||||
disabled: true,
|
||||
},
|
||||
@@ -224,7 +225,7 @@ export const useCommandPalette = ({
|
||||
}, [
|
||||
disabled,
|
||||
debouncedSearchText,
|
||||
searchQuery,
|
||||
trimmedQuery,
|
||||
isSearchLoading,
|
||||
searchError,
|
||||
searchResults,
|
||||
@@ -306,7 +307,8 @@ export const useCommandPalette = ({
|
||||
return {
|
||||
searchRequestId,
|
||||
searchResults,
|
||||
searchTerm: debouncedSearchText,
|
||||
liveSearchTerm: trimmedQuery,
|
||||
debouncedSearchTerm: debouncedSearchText,
|
||||
};
|
||||
};
|
||||
|
||||
|
||||
@@ -18,7 +18,7 @@ const BASIC_ACTION_ORDER_BY_NAME = BASIC_ACTION_ORDER.reduce<
|
||||
|
||||
export const processResults = (
|
||||
results: (string | PaletteActionImpl)[],
|
||||
searchTerm: string,
|
||||
hasSearchTerm: boolean,
|
||||
): (string | PaletteActionImpl)[] => {
|
||||
const groupedResults = _.groupBy(
|
||||
results.filter((r): r is PaletteActionImpl => !(typeof r === "string")),
|
||||
@@ -35,7 +35,7 @@ export const processResults = (
|
||||
const admin = processSection(t`Admin`, groupedResults["admin"]);
|
||||
const docs = processSection(t`Documentation`, groupedResults["docs"]);
|
||||
|
||||
if (searchTerm.trim().length === 0) {
|
||||
if (!hasSearchTerm) {
|
||||
return [...recent];
|
||||
}
|
||||
|
||||
|
||||
@@ -73,25 +73,25 @@ describe("command palette utils", () => {
|
||||
const testDoc = [createMockAction({ name: "doc action", section: "docs" })];
|
||||
|
||||
it("should limit to 6 actions including header", () => {
|
||||
const result = processResults(testActions, "xyz");
|
||||
const result = processResults(testActions, true);
|
||||
expect(result).toHaveLength(6);
|
||||
expect(result[0]).toBe("Actions");
|
||||
});
|
||||
|
||||
it("should not limit the other action types", () => {
|
||||
expect(
|
||||
processResults([...testActions, ...testSearch], "xyz"),
|
||||
processResults([...testActions, ...testSearch], true),
|
||||
).toHaveLength(12);
|
||||
expect(
|
||||
processResults([...testActions, ...testRecent], "xyz"),
|
||||
processResults([...testActions, ...testRecent], true),
|
||||
).toHaveLength(12);
|
||||
expect(
|
||||
processResults([...testRecent, ...testAdmin, ...testDoc], "xyz"),
|
||||
processResults([...testRecent, ...testAdmin, ...testDoc], true),
|
||||
).toHaveLength(19);
|
||||
});
|
||||
|
||||
it("should add a header to doc", () => {
|
||||
expect(processResults([...testDoc], "xyz")).toHaveLength(2);
|
||||
expect(processResults([...testDoc], true)).toHaveLength(2);
|
||||
});
|
||||
|
||||
it("should enforce a specific order", () => {
|
||||
@@ -103,7 +103,7 @@ describe("command palette utils", () => {
|
||||
...testActions,
|
||||
...testRecent,
|
||||
],
|
||||
"xyz",
|
||||
true,
|
||||
);
|
||||
|
||||
const actionsIndex = results.findIndex((action) => action === "Actions");
|
||||
@@ -121,7 +121,7 @@ describe("command palette utils", () => {
|
||||
});
|
||||
|
||||
it("should not show actions if there is no search query", () => {
|
||||
const result = processResults(testActions, "");
|
||||
const result = processResults(testActions, false);
|
||||
expect(result).toHaveLength(0);
|
||||
});
|
||||
|
||||
@@ -151,7 +151,7 @@ describe("command palette utils", () => {
|
||||
createMockAction({ id, name: id }),
|
||||
);
|
||||
|
||||
const result = processResults(actionsList, "xyz");
|
||||
const result = processResults(actionsList, true);
|
||||
expect(result).toEqual([
|
||||
"Actions",
|
||||
createMockAction({
|
||||
|
||||
@@ -724,7 +724,13 @@ index 0d3b4e7..0571622 100644
|
||||
minMatchCharLength: 1,
|
||||
};
|
||||
function order(a, b) {
|
||||
@@ -195,7 +199,7 @@ function useInternalMatches(filtered, search, fuse) {
|
||||
@@ -190,12 +194,12 @@ function useInternalMatches(filtered, search, fuse) {
|
||||
}
|
||||
var matches = [];
|
||||
// Use Fuse's `search` method to perform the search efficiently
|
||||
- var searchResults = fuse.search(throttledSearch);
|
||||
+ var searchResults = fuse.search(throttledSearch.trim());
|
||||
// Format the search results to match the existing structure
|
||||
matches = searchResults.map(function (_a) {
|
||||
var action = _a.item, score = _a.score;
|
||||
return ({
|
||||
|
||||
Reference in New Issue
Block a user