From 43b596ec6564fe98965eafddaf1eb7d7283079f3 Mon Sep 17 00:00:00 2001
From: Ryan Laurie <30528226+iethree@users.noreply.github.com>
Date: Wed, 28 Jun 2023 12:20:49 -0600
Subject: [PATCH] Indexed entity performance improvement (#31825)
* don't navigate away from the current model if the user searches for an item in that model
---
.../scenarios/models/model-indexes.cy.spec.js | 50 +++++++++++++++++
.../src/metabase/nav/components/SearchBar.tsx | 56 +++++++++++--------
.../metabase/nav/components/SearchResults.jsx | 30 ++++++----
.../search/components/SearchResult.styled.tsx | 1 +
.../search/components/SearchResult.tsx | 1 -
.../visualizations/LinkViz/LinkViz.tsx | 1 +
6 files changed, 105 insertions(+), 34 deletions(-)
diff --git a/e2e/test/scenarios/models/model-indexes.cy.spec.js b/e2e/test/scenarios/models/model-indexes.cy.spec.js
index 07ae4dede3c..c3e28b0be4f 100644
--- a/e2e/test/scenarios/models/model-indexes.cy.spec.js
+++ b/e2e/test/scenarios/models/model-indexes.cy.spec.js
@@ -21,6 +21,7 @@ describe("scenarios > model indexes", () => {
cy.intercept("POST", "/api/model-index").as("modelIndexCreate");
cy.intercept("DELETE", "/api/model-index/*").as("modelIndexDelete");
cy.intercept("PUT", "/api/card/*").as("cardUpdate");
+ cy.intercept("GET", "/api/card/*").as("cardGet");
cy.createQuestion({
name: "Products Model",
@@ -169,6 +170,50 @@ describe("scenarios > model indexes", () => {
cy.findAllByText("Anais Zieme").should("have.length", 2);
});
});
+
+ it("should not reload the model for record in the same model", () => {
+ createModelIndex({ modelId, pkName: "ID", valueName: "TITLE" });
+
+ cy.visit("/");
+
+ cy.findByTestId("app-bar")
+ .findByPlaceholderText("Search…")
+ .type("marble shoes");
+
+ cy.wait("@searchQuery");
+
+ cy.findByTestId("search-results-list")
+ .findByText("Small Marble Shoes")
+ .click();
+
+ cy.wait("@dataset");
+
+ cy.findByTestId("object-detail").within(() => {
+ cy.findByText("Product");
+ cy.findByText("Small Marble Shoes");
+ cy.findByText("Doohickey");
+ });
+
+ // for some reason we hit this endpoint twice on initial load
+ expectCardQueries(2);
+
+ cy.get("body").type("{esc}");
+
+ cy.findByTestId("app-bar")
+ .findByPlaceholderText("Search…")
+ .clear()
+ .type("silk coat");
+
+ cy.findByTestId("search-results-list")
+ .findByText("Ergonomic Silk Coat")
+ .click();
+
+ cy.findByTestId("object-detail").within(() => {
+ cy.findByText("Upton, Kovacek and Halvorson");
+ });
+
+ expectCardQueries(2);
+ });
});
function editTitleMetadata() {
@@ -206,3 +251,8 @@ function createModelIndex({ modelId, pkName, valueName }) {
},
);
}
+
+const expectCardQueries = num =>
+ cy.get("@cardGet.all").then(interceptions => {
+ expect(interceptions).to.have.length(num);
+ });
diff --git a/frontend/src/metabase/nav/components/SearchBar.tsx b/frontend/src/metabase/nav/components/SearchBar.tsx
index 882eba0c738..0edc204c3d5 100644
--- a/frontend/src/metabase/nav/components/SearchBar.tsx
+++ b/frontend/src/metabase/nav/components/SearchBar.tsx
@@ -1,7 +1,5 @@
import { MouseEvent, useEffect, useCallback, useRef, useState } from "react";
-import _ from "underscore";
import { t } from "ttag";
-import { connect } from "react-redux";
import { push } from "react-router-redux";
import { withRouter } from "react-router";
import { Location, LocationDescriptorObject } from "history";
@@ -14,6 +12,8 @@ import { useOnClickOutside } from "metabase/hooks/use-on-click-outside";
import { useToggle } from "metabase/hooks/use-toggle";
import { isSmallScreen } from "metabase/lib/dom";
import MetabaseSettings from "metabase/lib/settings";
+import { useDispatch } from "metabase/lib/redux";
+import { zoomInRow } from "metabase/query_builder/actions";
import SearchResults from "./SearchResults";
import RecentsList from "./RecentsList";
@@ -35,20 +35,12 @@ type RouterProps = {
location: SearchAwareLocation;
};
-type DispatchProps = {
- onChangeLocation: (nextLocation: LocationDescriptorObject) => void;
-};
-
type OwnProps = {
onSearchActive?: () => void;
onSearchInactive?: () => void;
};
-type Props = RouterProps & DispatchProps & OwnProps;
-
-const mapDispatchToProps = {
- onChangeLocation: push,
-};
+type Props = RouterProps & OwnProps;
function isSearchPageLocation(location: Location) {
const components = location.pathname.split("/");
@@ -62,12 +54,7 @@ function getSearchTextFromLocation(location: SearchAwareLocation) {
return "";
}
-function SearchBar({
- location,
- onSearchActive,
- onSearchInactive,
- onChangeLocation,
-}: Props) {
+function SearchBarView({ location, onSearchActive, onSearchInactive }: Props) {
const [searchText, setSearchText] = useState<string>(() =>
getSearchTextFromLocation(location),
);
@@ -79,6 +66,12 @@ function SearchBar({
const previousLocation = usePrevious(location);
const container = useRef<HTMLDivElement>(null);
const searchInput = useRef<HTMLInputElement>(null);
+ const dispatch = useDispatch();
+
+ const onChangeLocation = useCallback(
+ (nextLocation: LocationDescriptorObject) => dispatch(push(nextLocation)),
+ [dispatch],
+ );
const onInputContainerClick = useCallback(() => {
searchInput.current?.focus();
@@ -89,6 +82,19 @@ function SearchBar({
setSearchText(e.target.value);
}, []);
+ const onSearchItemSelect = useCallback(
+ result => {
+ // if we're already looking at the right model, don't navigate, just update the zoomed in row
+ const isSameModel = result?.model_id === location?.state?.cardId;
+ if (isSameModel && result.model === "indexed-entity") {
+ zoomInRow({ objectId: result.id })(dispatch);
+ } else {
+ onChangeLocation(result.getUrl());
+ }
+ },
+ [dispatch, onChangeLocation, location?.state?.cardId],
+ );
+
useOnClickOutside(container, setInactive);
useKeyboardShortcut("Escape", setInactive);
@@ -181,7 +187,10 @@ function SearchBar({
<SearchResultsFloatingContainer>
{hasSearchText ? (
<SearchResultsContainer>
- <SearchResults searchText={searchText.trim()} />
+ <SearchResults
+ searchText={searchText.trim()}
+ onEntitySelect={onSearchItemSelect}
+ />
</SearchResultsContainer>
) : (
<RecentsList />
@@ -191,8 +200,9 @@ function SearchBar({
</SearchBarRoot>
);
}
-// eslint-disable-next-line import/no-default-export -- deprecated usage
-export default _.compose(
- withRouter,
- connect(null, mapDispatchToProps),
-)(SearchBar);
+
+export const SearchBar = withRouter(SearchBarView);
+
+// for some reason our unit test don't work if this is a name export ¯\_(ツ)_/¯
+// eslint-disable-next-line import/no-default-export
+export default SearchBar;
diff --git a/frontend/src/metabase/nav/components/SearchResults.jsx b/frontend/src/metabase/nav/components/SearchResults.jsx
index 6bc14497de2..e31b52f343f 100644
--- a/frontend/src/metabase/nav/components/SearchResults.jsx
+++ b/frontend/src/metabase/nav/components/SearchResults.jsx
@@ -16,6 +16,7 @@ const propTypes = {
list: PropTypes.array,
onChangeLocation: PropTypes.func,
onEntitySelect: PropTypes.func,
+ forceEntitySelect: PropTypes.bool,
searchText: PropTypes.string,
};
@@ -23,6 +24,7 @@ const SearchResults = ({
list,
onChangeLocation,
onEntitySelect,
+ forceEntitySelect,
searchText,
}) => {
const { reset, getRef, cursorIndex } = useListKeyboardNavigation({
@@ -42,16 +44,24 @@ const SearchResults = ({
return (
<ul data-testid="search-results-list">
{hasResults ? (
- list.map((item, index) => (
- <li key={`${item.model}:${item.id}`} ref={getRef(item)}>
- <SearchResult
- result={item}
- compact={true}
- isSelected={cursorIndex === index}
- onClick={onEntitySelect}
- />
- </li>
- ))
+ list.map((item, index) => {
+ const isIndexedEntity = item.model === "indexed-entity";
+ const onClick =
+ onEntitySelect && (isIndexedEntity || forceEntitySelect)
+ ? onEntitySelect
+ : undefined;
+
+ return (
+ <li key={`${item.model}:${item.id}`} ref={getRef(item)}>
+ <SearchResult
+ result={item}
+ compact={true}
+ isSelected={cursorIndex === index}
+ onClick={onClick}
+ />
+ </li>
+ );
+ })
) : (
<EmptyStateContainer>
<EmptyState message={t`Didn't find anything`} icon="search" />
diff --git a/frontend/src/metabase/search/components/SearchResult.styled.tsx b/frontend/src/metabase/search/components/SearchResult.styled.tsx
index cbb5884affe..4dcf3595463 100644
--- a/frontend/src/metabase/search/components/SearchResult.styled.tsx
+++ b/frontend/src/metabase/search/components/SearchResult.styled.tsx
@@ -73,6 +73,7 @@ export const ResultButton = styled.button<ResultStylesProps>`
padding-right: 0.5rem;
text-align: left;
cursor: pointer;
+ width: 100%;
&:hover {
${Title} {
color: ${color("brand")};
diff --git a/frontend/src/metabase/search/components/SearchResult.tsx b/frontend/src/metabase/search/components/SearchResult.tsx
index 116b48f4011..5c2b98044f1 100644
--- a/frontend/src/metabase/search/components/SearchResult.tsx
+++ b/frontend/src/metabase/search/components/SearchResult.tsx
@@ -1,6 +1,5 @@
import { color } from "metabase/lib/colors";
import { isSyncCompleted } from "metabase/lib/syncing";
-
import { Icon } from "metabase/core/components/Icon";
import Text from "metabase/components/type/Text";
diff --git a/frontend/src/metabase/visualizations/visualizations/LinkViz/LinkViz.tsx b/frontend/src/metabase/visualizations/visualizations/LinkViz/LinkViz.tsx
index cbc4ad4d737..c88cafba836 100644
--- a/frontend/src/metabase/visualizations/visualizations/LinkViz/LinkViz.tsx
+++ b/frontend/src/metabase/visualizations/visualizations/LinkViz/LinkViz.tsx
@@ -149,6 +149,7 @@ function LinkViz({
<SearchResultsContainer>
<SearchResults
searchText={url?.trim()}
+ forceEntitySelect
onEntitySelect={handleEntitySelect}
models={MODELS_TO_SEARCH}
/>
--
GitLab