Fix redundant network requests triggered in object detail modal (#33710)

* Extract ObjectDetailBody to a separate file

* Extract ObjectDetailHeader to a separate file

* Extract ObjectDetailView to a separate file
- extract ObjectDetailWrapper.styled.tsx

* Extract ObjectDetailHeader unit tests

* Extract ObjectDetailBody unit tests

* Rename ObjectDetail.unit.spec.tsx to ObjectDetailView.unit.spec.tsx

* Remove the non-null assertion operator

* Use relative import

* Introduce hasFks

* Do not loadFKReferences() when there was no previous zoomed row
- this function will be called in useMount hook's callback

* Use project-wide != null pattern

* Remove loadFKReferences() on mount because it is duplicated later by useEffect

* Extract ObjectRelationships.styled

* Extract ObjectDetailTable.styled

* Fix imports

* Add repro test for #32720

e2e/test/scenarios/visualizations/object_detail.cy.spec.js

@@ -181,6 +181,24 @@ describe("scenarios > question > object details", () => {
     cy.findByText(`Showing ${EXPECTED_LINKED_ORDERS_COUNT} rows`);
   });
 
+  it("should fetch linked entities data only once per entity type when reopening the modal (metabase#32720)", () => {
+    cy.intercept("POST", "/api/dataset", cy.spy().as("fetchDataset"));
+
+    openProductsTable();
+    cy.get("@fetchDataset").should("have.callCount", 1);
+
+    drillPK({ id: 5 });
+    cy.get("@fetchDataset").should("have.callCount", 3);
+
+    cy.findByTestId("object-detail-close-button").click();
+
+    drillPK({ id: 5 });
+    cy.get("@fetchDataset").should("have.callCount", 5);
+
+    cy.wait(100);
+    cy.get("@fetchDataset").should("have.callCount", 5);
+  });
+
   it("should not offer drill-through on the object detail records (metabase#20560)", () => {
     openPeopleTable({ limit: 2 });
 

frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetail.tsx

@@ -20,9 +20,9 @@ import {
   getZoomedObjectId,
 } from "metabase/query_builder/selectors";
 import { getUser } from "metabase/selectors/user";
-import { ObjectDetailWrapper } from "metabase/visualizations/components/ObjectDetail/ObjectDetailWrapper";
 import type ForeignKey from "metabase-lib/metadata/ForeignKey";
 
+import { ObjectDetailWrapper } from "./ObjectDetailWrapper";
 import type { ObjectDetailProps, ObjectId } from "./types";
 import { getIdValue, getSingleResultsRow } from "./utils";
 
@@ -46,9 +46,7 @@ const mapStateToProps = (state: State, { data }: ObjectDetailProps) => {
   const canZoomNextRow = isZooming ? Boolean(getCanZoomNextRow(state)) : false;
 
   return {
-    // FIXME: remove the non-null assertion operator
-    // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-    question: getQuestion(state)!,
+    question: getQuestion(state),
     table,
     tableForeignKeys: getTableForeignKeys(state),
     tableForeignKeyReferences: getTableForeignKeyReferences(state),

frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetailView.styled.tsx

 import styled from "@emotion/styled";
 
-import { color } from "metabase/lib/colors";
-
 interface ObjectDetailContainerProps {
   wide: boolean;
 }
@@ -10,70 +8,16 @@ export const ObjectDetailContainer = styled.div<ObjectDetailContainerProps>`
   overflow-y: auto;
   height: 100%;
 `;
+
 export const ObjectDetailWrapperDiv = styled.div`
   height: 100%;
   display: flex;
   flex-direction: column;
 `;
 
-export const ObjectDetailsTable = styled.div`
-  overflow-y: auto;
-  flex: 1;
-  padding: 2rem;
-`;
-
-export const ObjectRelationships = styled.div`
-  overflow-y: auto;
-  flex: 0 0 100%;
-  padding: 2rem;
-  background-color: ${color("bg-light")};
-`;
-
 export const ErrorWrapper = styled.div`
   height: 480px;
   display: flex;
   justify-content: center;
   align-items: center;
 `;
-
-type GridContainerProps = { cols?: number };
-
-export const GridContainer = styled.div<GridContainerProps>`
-  display: grid;
-  grid-template-columns: repeat(${props => props.cols || 2}, minmax(0, 1fr));
-  gap: 1rem;
-`;
-
-export interface GridItemProps {
-  colSpan?: number;
-}
-
-export const GridCell = styled.div<GridItemProps>`
-  grid-column: span ${props => props.colSpan || 1} / span
-    ${props => props.colSpan || 1};
-`;
-
-export const FitImage = styled.img`
-  max-width: 100%;
-  max-height: 18rem;
-  object-fit: contain;
-  margin: 1rem auto;
-`;
-
-export interface ObjectRelationshipContentProps {
-  isClickable: boolean;
-}
-
-export const ObjectRelationContent = styled.div<ObjectRelationshipContentProps>`
-  display: flex;
-  align-items: center;
-  margin: 1rem 0;
-  padding-bottom: 1rem;
-  border-bottom: 1px solid ${color("border")};
-  color: ${props => color(props.isClickable ? "text-dark" : "text-medium")};
-  cursor: ${props => props.isClickable && "pointer"};
-
-  &:hover {
-    color: ${props => props.isClickable && color("brand")};
-  }
-`;

frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetailView.tsx

@@ -24,13 +24,13 @@ import { isVirtualCardId } from "metabase-lib/metadata/utils/saved-questions";
 import type ForeignKey from "metabase-lib/metadata/ForeignKey";
 
 import { DeleteObjectModal } from "./DeleteObjectModal";
+import { ObjectDetailBody } from "./ObjectDetailBody";
+import { ObjectDetailHeader } from "./ObjectDetailHeader";
 import {
   ErrorWrapper,
   ObjectDetailContainer,
   ObjectDetailWrapperDiv,
 } from "./ObjectDetailView.styled";
-import { ObjectDetailBody } from "./ObjectDetailBody";
-import { ObjectDetailHeader } from "./ObjectDetailHeader";
 import type { ObjectDetailProps } from "./types";
 import {
   getActionItems,
@@ -77,6 +77,10 @@ export function ObjectDetailView({
   const isDeleteModalOpen = typeof deleteActionId === "number";
   const isModalOpen = isActionExecuteModalOpen || isDeleteModalOpen;
 
+  const hasPk = !!data.cols.find(isPK);
+  const hasFks = !_.isEmpty(tableForeignKeys);
+  const hasRelationships = showRelations && hasFks && hasPk;
+
   const handleExecuteModalClose = () => {
     setActionId(undefined);
   };
@@ -116,10 +120,6 @@ export function ObjectDetailView({
     if (table && _.isEmpty(table.fks) && !isVirtualCardId(table.id)) {
       fetchTableFks(table.id as ConcreteTableId);
     }
-    // load up FK references
-    if (!_.isEmpty(tableForeignKeys)) {
-      loadFKReferences();
-    }
   });
 
   useEffect(() => {
@@ -176,10 +176,12 @@ export function ObjectDetailView({
   }, [maybeLoading, passedData, question, zoomedRowID, pkIndex]);
 
   useEffect(() => {
-    if (!_.isEmpty(tableForeignKeys) && prevZoomedRowId !== zoomedRowID) {
+    const hadPrevZoomedRow = prevZoomedRowId != null;
+
+    if (hasFks && hadPrevZoomedRow && prevZoomedRowId !== zoomedRowID) {
       loadFKReferences();
     }
-  }, [tableForeignKeys, prevZoomedRowId, zoomedRowID, loadFKReferences]);
+  }, [hasFks, prevZoomedRowId, zoomedRowID, loadFKReferences]);
 
   useEffect(() => {
     const queryCompleted = !prevData && data;
@@ -191,18 +193,13 @@ export function ObjectDetailView({
 
   useEffect(() => {
     // if the card changed or table metadata loaded then reload fk references
-    const tableFKsJustLoaded =
-      _.isEmpty(prevTableForeignKeys) && !_.isEmpty(tableForeignKeys);
-    if (data !== prevData || tableFKsJustLoaded) {
+    const tableFKsJustLoaded = _.isEmpty(prevTableForeignKeys) && hasFks;
+    const hasCardChanged = data !== prevData;
+
+    if (hasCardChanged || tableFKsJustLoaded) {
       loadFKReferences();
     }
-  }, [
-    tableForeignKeys,
-    data,
-    prevData,
-    prevTableForeignKeys,
-    loadFKReferences,
-  ]);
+  }, [hasFks, data, prevData, prevTableForeignKeys, loadFKReferences]);
 
   const onFollowForeignKey = useCallback(
     (fk: ForeignKey) => {
@@ -282,10 +279,6 @@ export function ObjectDetailView({
     settings,
   });
 
-  const hasPk = !!data.cols.find(isPK);
-  const hasRelationships =
-    showRelations && !_.isEmpty(tableForeignKeys) && hasPk;
-
   return (
     <>
       <ObjectDetailContainer wide={hasRelationships} className={className}>

frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetailWrapper.styled.tsx

@@ -7,11 +7,9 @@ import { breakpointMinMedium } from "metabase/styled-components/theme/media-quer
 import TableFooter from "../TableSimple/TableFooter";
 
 import { ObjectDetailBodyWrapper } from "./ObjectDetailBody.styled";
-import {
-  ObjectDetailContainer,
-  ObjectDetailsTable,
-  ObjectRelationships,
-} from "./ObjectDetailView.styled";
+import { ObjectDetailContainer } from "./ObjectDetailView.styled";
+import { ObjectDetailsTable } from "./ObjectDetailsTable.styled";
+import { ObjectRelationships } from "./ObjectRelationships.styled";
 
 export const RootModal = styled(Modal)`
   ${ObjectDetailContainer} {

frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetailsTable.styled.tsx

+import styled from "@emotion/styled";
+
+export const ObjectDetailsTable = styled.div`
+  overflow-y: auto;
+  flex: 1;
+  padding: 2rem;
+`;
+
+type GridContainerProps = { cols?: number };
+
+export const GridContainer = styled.div<GridContainerProps>`
+  display: grid;
+  grid-template-columns: repeat(${props => props.cols || 2}, minmax(0, 1fr));
+  gap: 1rem;
+`;
+
+export interface GridItemProps {
+  colSpan?: number;
+}
+
+export const GridCell = styled.div<GridItemProps>`
+  grid-column: span ${props => props.colSpan || 1} / span
+    ${props => props.colSpan || 1};
+`;
+
+export const FitImage = styled.img`
+  max-width: 100%;
+  max-height: 18rem;
+  object-fit: contain;
+  margin: 1rem auto;
+`;

frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetailsTable.tsx

@@ -24,7 +24,7 @@ import {
   GridContainer,
   GridCell,
   FitImage,
-} from "./ObjectDetailView.styled";
+} from "./ObjectDetailsTable.styled";
 
 export interface DetailsTableCellProps {
   column: any;

frontend/src/metabase/visualizations/components/ObjectDetail/ObjectRelationships.styled.tsx

+import styled from "@emotion/styled";
+
+import { color } from "metabase/lib/colors";
+
+export const ObjectRelationships = styled.div`
+  overflow-y: auto;
+  flex: 0 0 100%;
+  padding: 2rem;
+  background-color: ${color("bg-light")};
+`;
+
+export interface ObjectRelationshipContentProps {
+  isClickable: boolean;
+}
+
+export const ObjectRelationContent = styled.div<ObjectRelationshipContentProps>`
+  display: flex;
+  align-items: center;
+  margin: 1rem 0;
+  padding-bottom: 1rem;
+  border-bottom: 1px solid ${color("border")};
+  color: ${props => color(props.isClickable ? "text-dark" : "text-medium")};
+  cursor: ${props => props.isClickable && "pointer"};
+
+  &:hover {
+    color: ${props => props.isClickable && color("brand")};
+  }
+`;

frontend/src/metabase/visualizations/components/ObjectDetail/ObjectRelationships.tsx

@@ -12,7 +12,7 @@ import type { ForeignKeyReferences } from "./types";
 import {
   ObjectRelationContent,
   ObjectRelationships,
-} from "./ObjectDetailView.styled";
+} from "./ObjectRelationships.styled";
 
 export interface RelationshipsProps {
   objectName: string;