optimize loading questions only for active tab (#31578)

* only load cards and metadata for current dashboard tab

* add e2e test

* refactor

* more refactoring

* fix filters not working

* update e2e test

* fix bug with x-rays

* add dashboard_tab_id to automagic-dashboard response

* use schema check instead

* fix on public dashboards

* remove FE x-ray fix now that BE is fixed

* add test for public dashboards

* fix duplicate request for public dashboards

* fix notification bug

* fix failing unit tests in `reducers.unit.spec.js`

---------

Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com>

e2e/support/helpers/e2e-misc-helpers.js

@@ -169,7 +169,13 @@ export function visitDashboard(dashboard_id, { params = {} } = {}) {
     cy.intercept("GET", `/api/dashboard/${dashboard_id}`).as(dashboardAlias);
 
     const canViewDashboard = hasAccess(status);
-    const validQuestions = dashboardHasQuestions(ordered_cards);
+
+    let validQuestions = dashboardHasQuestions(ordered_cards);
+    if (params.tab != null) {
+      validQuestions = validQuestions.filter(
+        card => card.dashboard_tab_id === params.tab,
+      );
+    }
 
     if (canViewDashboard && validQuestions) {
       // If dashboard has valid questions (GUI or native),

e2e/test/scenarios/dashboard/tabs.cy.spec.js

@@ -78,6 +78,72 @@ describe("scenarios > dashboard > tabs", () => {
       cy.findByText("Our analytics").should("be.visible");
     });
   });
+
+  it("should only fetch cards on the current tab", () => {
+    visitDashboardAndCreateTab({ dashboardId: 1, save: false });
+
+    // Add card to second tab
+    cy.icon("pencil").click();
+    openQuestionsSidebar();
+    sidebar().within(() => {
+      cy.findByText("Orders, Count").click();
+    });
+    saveDashboard();
+
+    cy.intercept(
+      "POST",
+      `/api/dashboard/1/dashcard/1/card/1/query`,
+      cy.spy().as("firstTabQuery"),
+    );
+    cy.intercept(
+      "POST",
+      `/api/dashboard/1/dashcard/2/card/2/query`,
+      cy.spy().as("secondTabQuery"),
+    );
+
+    // Visit first tab and confirm only first card was queried
+    visitDashboard(1, { params: { tab: 1 } });
+    cy.get("@firstTabQuery").should("have.been.calledOnce");
+    cy.get("@secondTabQuery").should("not.have.been.called");
+
+    // Visit second tab and confirm only second card was queried
+    cy.findByRole("tab", { name: "Tab 2" }).click();
+    cy.get("@firstTabQuery").should("have.been.calledOnce");
+    cy.get("@secondTabQuery").should("have.been.calledOnce");
+
+    // Go back to first tab, expect no additional queries
+    cy.findByRole("tab", { name: "Tab 1" }).click();
+    cy.get("@firstTabQuery").should("have.been.calledOnce");
+    cy.get("@secondTabQuery").should("have.been.calledOnce");
+
+    // Go to public dashboard
+    cy.request("PUT", "/api/setting/enable-public-sharing", { value: true });
+    cy.request("POST", `/api/dashboard/1/public_link`).then(
+      ({ body: { uuid } }) => {
+        cy.intercept(
+          "GET",
+          `/api/public/dashboard/${uuid}/dashcard/1/card/1?parameters=%5B%5D`,
+          cy.spy().as("publicFirstTabQuery"),
+        );
+        cy.intercept(
+          "GET",
+          `/api/public/dashboard/${uuid}/dashcard/2/card/2?parameters=%5B%5D`,
+          cy.spy().as("publicSecondTabQuery"),
+        );
+
+        cy.visit(`public/dashboard/${uuid}`);
+      },
+    );
+
+    // Check first tab requests
+    cy.get("@publicFirstTabQuery").should("have.been.calledOnce");
+    cy.get("@publicSecondTabQuery").should("not.have.been.called");
+
+    // Visit second tab and confirm only second card was queried
+    cy.findByRole("tab", { name: "Tab 2" }).click();
+    cy.get("@publicFirstTabQuery").should("have.been.calledOnce");
+    cy.get("@publicSecondTabQuery").should("have.been.calledOnce");
+  });
 });
 
 describeWithSnowplow("scenarios > dashboard > tabs", () => {

frontend/src/metabase/dashboard/actions/data-fetching.js

@@ -34,6 +34,7 @@ import {
   getCanShowAutoApplyFiltersToast,
   getDashboardById,
   getDashCardById,
+  getSelectedTabId,
 } from "../selectors";
 
 import {
@@ -60,6 +61,9 @@ export const FETCH_DASHBOARD_CARD_DATA =
 export const CANCEL_FETCH_DASHBOARD_CARD_DATA =
   "metabase/dashboard/CANCEL_FETCH_DASHBOARD_CARD_DATA";
 
+export const FETCH_DASHBOARD_CARD_METADATA =
+  "metabase/dashboard/FETCH_DASHBOARD_CARD_METADATA";
+
 export const FETCH_CARD_DATA = "metabase/dashboard/FETCH_CARD_DATA";
 export const CANCEL_FETCH_CARD_DATA =
   "metabase/dashboard/CANCEL_FETCH_CARD_DATA";
@@ -197,7 +201,16 @@ export const fetchDashboard = createThunkAction(
       }
 
       if (dashboardType === "normal" || dashboardType === "transient") {
-        await dispatch(loadMetadataForDashboard(result.ordered_cards));
+        const selectedTabId = getSelectedTabId(getState());
+
+        const cards =
+          selectedTabId === undefined
+            ? result.ordered_cards
+            : result.ordered_cards.filter(
+                c => c.dashboard_tab_id === selectedTabId,
+              );
+
+        await dispatch(loadMetadataForDashboard(cards));
       }
 
       const isUsingCachedResults = entities != null;
@@ -409,14 +422,23 @@ export const fetchDashboardCardData = createThunkAction(
   FETCH_DASHBOARD_CARD_DATA,
   options => (dispatch, getState) => {
     const dashboard = getDashboardComplete(getState());
+    const selectedTabId = getSelectedTabId(getState());
+    const dashcardIds = [];
 
     const promises = getAllDashboardCards(dashboard)
       .map(({ card, dashcard }) => {
-        if (!isVirtualDashCard(dashcard)) {
-          return dispatch(fetchCardData(card, dashcard, options)).then(() => {
-            return dispatch(updateLoadingTitle());
-          });
+        if (
+          isVirtualDashCard(dashcard) ||
+          (selectedTabId !== undefined &&
+            dashcard.dashboard_tab_id !== selectedTabId)
+        ) {
+          return;
         }
+
+        dashcardIds.push(dashcard.id);
+        return dispatch(fetchCardData(card, dashcard, options)).then(() => {
+          return dispatch(updateLoadingTitle());
+        });
       })
       .filter(p => !!p);
 
@@ -428,7 +450,21 @@ export const fetchDashboardCardData = createThunkAction(
       dispatch(loadingComplete());
     });
 
-    return { currentTime: performance.now() };
+    return { currentTime: performance.now(), dashcardIds };
+  },
+);
+
+export const fetchDashboardCardMetadata = createThunkAction(
+  FETCH_DASHBOARD_CARD_METADATA,
+  () => async (dispatch, getState) => {
+    const allDashCards = getDashboardComplete(getState()).ordered_cards;
+    const selectedTabId = getSelectedTabId(getState());
+
+    const cards = allDashCards.filter(
+      dc =>
+        selectedTabId !== undefined && dc.dashboard_tab_id === selectedTabId,
+    );
+    await dispatch(loadMetadataForDashboard(cards));
   },
 );
 

frontend/src/metabase/dashboard/components/Dashboard/Dashboard.jsx

@@ -59,6 +59,7 @@ class Dashboard extends Component {
 
     dashboard: PropTypes.object,
     dashboardId: PropTypes.number,
+    dashcardData: PropTypes.object,
     selectedTabId: PropTypes.number,
     parameters: PropTypes.array,
     parameterValues: PropTypes.object,
@@ -73,6 +74,7 @@ class Dashboard extends Component {
     cancelFetchDashboardCardData: PropTypes.func.isRequired,
     fetchDashboard: PropTypes.func.isRequired,
     fetchDashboardCardData: PropTypes.func.isRequired,
+    fetchDashboardCardMetadata: PropTypes.func.isRequired,
     initialize: PropTypes.func.isRequired,
     onRefreshPeriodChange: PropTypes.func,
     saveDashboardAndCards: PropTypes.func.isRequired,
@@ -140,7 +142,16 @@ class Dashboard extends Component {
     if (prevProps.dashboardId !== this.props.dashboardId) {
       await this.loadDashboard(this.props.dashboardId);
       this.throttleParameterWidgetStickiness();
-    } else if (
+      return;
+    }
+
+    if (!_.isEqual(prevProps.selectedTabId, this.props.selectedTabId)) {
+      this.props.fetchDashboardCardData();
+      this.props.fetchDashboardCardMetadata();
+      return;
+    }
+
+    if (
       !_.isEqual(prevProps.parameterValues, this.props.parameterValues) ||
       (!prevProps.dashboard && this.props.dashboard)
     ) {

frontend/src/metabase/dashboard/reducers.js

@@ -43,8 +43,9 @@ import {
   UNDO_REMOVE_CARD_FROM_DASH,
   SHOW_AUTO_APPLY_FILTERS_TOAST,
   tabsReducer,
+  SET_LOADING_DASHCARDS_COMPLETE,
 } from "./actions";
-import { isVirtualDashCard, syncParametersAndEmbeddingParams } from "./utils";
+import { syncParametersAndEmbeddingParams } from "./utils";
 import { INITIAL_DASHBOARD_STATE } from "./constants";
 
 const dashboardId = handleActions(
@@ -367,26 +368,19 @@ const loadingDashCards = handleActions(
         loadingStatus: "idle",
       }),
     },
-    [FETCH_DASHBOARD]: {
-      next: (state, { payload }) => {
-        const cardIds = Object.values(payload.entities.dashcard || {})
-          .filter(dc => !isVirtualDashCard(dc))
-          .map(dc => dc.id);
+    [FETCH_DASHBOARD_CARD_DATA]: {
+      next: (state, { payload: { currentTime, dashcardIds } }) => {
+        const loadingIds = Array.isArray(dashcardIds) ? dashcardIds : [];
+
         return {
           ...state,
-          dashcardIds: cardIds,
-          loadingIds: cardIds,
-          loadingStatus: "idle",
+          dashcardIds: loadingIds,
+          loadingIds,
+          loadingStatus: loadingIds.length > 0 ? "running" : "idle",
+          startTime: loadingIds.length > 0 ? currentTime : null,
         };
       },
     },
-    [FETCH_DASHBOARD_CARD_DATA]: {
-      next: (state, { payload: { currentTime } }) => ({
-        ...state,
-        loadingStatus: state.loadingIds.length > 0 ? "running" : "idle",
-        startTime: state.loadingIds.length > 0 ? currentTime : null,
-      }),
-    },
     [FETCH_CARD_DATA]: {
       next: (state, { payload: { dashcard_id, currentTime } }) => {
         const loadingIds = state.loadingIds.filter(id => id !== dashcard_id);
@@ -409,6 +403,15 @@ const loadingDashCards = handleActions(
         };
       },
     },
+    [SET_LOADING_DASHCARDS_COMPLETE]: {
+      next: state => {
+        return {
+          ...state,
+          loadingIds: [],
+          loadingStatus: "complete",
+        };
+      },
+    },
     [RESET]: {
       next: state => ({
         ...state,

frontend/src/metabase/dashboard/reducers.unit.spec.js

@@ -251,7 +251,7 @@ describe("dashboard reducers", () => {
           },
           {
             type: FETCH_DASHBOARD_CARD_DATA,
-            payload: { currentTime: 100 },
+            payload: { currentTime: 100, dashcardIds },
           },
         ),
       ).toMatchObject({

frontend/src/metabase/public/containers/PublicDashboard.jsx

@@ -86,7 +86,9 @@ class PublicDashboard extends Component {
     initialize();
     try {
       await fetchDashboard(uuid || token, location.query);
-      await fetchDashboardCardData({ reload: false, clearCache: true });
+      if (this.props.dashboard.ordered_tabs.length === 0) {
+        await fetchDashboardCardData({ reload: false, clearCache: true });
+      }
     } catch (error) {
       console.error(error);
       setErrorPage(error);
@@ -106,6 +108,12 @@ class PublicDashboard extends Component {
       return this._initialize();
     }
 
+    if (!_.isEqual(prevProps.selectedTabId, this.props.selectedTabId)) {
+      this.props.fetchDashboardCardData();
+      this.props.fetchDashboardCardMetadata();
+      return;
+    }
+
     if (!_.isEqual(this.props.parameterValues, prevProps.parameterValues)) {
       this.props.fetchDashboardCardData({ reload: false, clearCache: true });
     }

src/metabase/automagic_dashboards/comparison.clj

@@ -94,16 +94,16 @@
               series (-> card-right
                          (update :name #(format "%s (%s)" % (comparison-name right)))
                          vector)]
-          (update dashboard :ordered_cards conj {:col                    0
-                                                 :row                    row
-                                                 :size_x                 populate/grid-width
-                                                 :size_y                 height
-                                                 :card                   card
-                                                 :card_id                (:id card)
-                                                 :series                 series
-                                                 :visualization_settings {:graph.y_axis.auto_split false
-                                                                          :graph.series_labels     [(:name card) (:name (first series))]}
-                                                 :id                     (gensym)}))
+          (update dashboard :ordered_cards conj (merge (populate/card-defaults)
+                                                       {:col                    0
+                                                        :row                    row
+                                                        :size_x                 populate/grid-width
+                                                        :size_y                 height
+                                                        :card                   card
+                                                        :card_id                (:id card)
+                                                        :series                 series
+                                                        :visualization_settings {:graph.y_axis.auto_split false
+                                                                                 :graph.series_labels     [(:name card) (:name (first series))]}})))
         (let [width        (/ populate/grid-width 2)
               series-left  (map clone-card (:series card-left))
               series-right (map clone-card (:series card-right))
@@ -114,24 +114,25 @@
                              (not (multiseries? card-right))
                              (assoc-in [:visualization_settings :graph.colors] [color-right]))]
           (-> dashboard
-              (update :ordered_cards conj {:col                    0
-                                           :row                    row
-                                           :size_x                 width
-                                           :size_y                 height
-                                           :card                   card-left
-                                           :card_id                (:id card-left)
-                                           :series                 series-left
-                                           :visualization_settings {}
-                                           :id                     (gensym)})
-              (update :ordered_cards conj {:col                    width
-                                           :row                    row
-                                           :size_x                 width
-                                           :size_y                 height
-                                           :card                   card-right
-                                           :card_id                (:id card-right)
-                                           :series                 series-right
-                                           :visualization_settings {}
-                                           :id                     (gensym)})))))
+              (update :ordered_cards conj (merge (populate/card-defaults)
+                                                 {:col                    0
+                                                  :row                    row
+                                                  :size_x                 width
+                                                  :size_y                 height
+                                                  :card                   card-left
+                                                  :card_id                (:id card-left)
+                                                  :series                 series-left
+                                                  :visualization_settings {}}))
+              (update :ordered_cards conj (merge (populate/card-defaults)
+                                                 {:col                    width
+                                                   :row                    row
+                                                   :size_x                 width
+                                                   :size_y                 height
+                                                   :card                   card-right
+                                                   :card_id                (:id card-right)
+                                                   :series                 series-right
+                                                   :visualization_settings {}}))))))
+
     (populate/add-text-card dashboard {:text                   (:text card)
                                        :width                  (/ populate/grid-width 2)
                                        :height                 (:height card)

src/metabase/automagic_dashboards/populate.clj

@@ -126,6 +126,14 @@
 
                                      y_label       (assoc :graph.y_axis.title_text y_label)))}))
 
+
+(defn card-defaults
+  "Default properties for a dashcard on magic dashboard."
+  []
+  {:id                     (gensym)
+   :dashboard_tab_id       nil
+   :visualization_settings {}})
+
 (defn- add-card
   "Add a card to dashboard `dashboard` at position [`x`, `y`]."
   [dashboard {:keys [title description dataset_query width height id] :as card} [x y]]
@@ -137,33 +145,34 @@
                   :id            (or id (gensym))}
                  (merge (visualization-settings card))
                  card/populate-query-fields)]
-    (update dashboard :ordered_cards conj {:col                    y
-                                           :row                    x
-                                           :size_x                 width
-                                           :size_y                 height
-                                           :card                   card
-                                           :card_id                (:id card)
-                                           :visualization_settings {}
-                                           :id                     (gensym)})))
+    (update dashboard :ordered_cards conj
+            (merge (card-defaults)
+             {:col                    y
+              :row                    x
+              :size_x                 width
+              :size_y                 height
+              :card                   card
+              :card_id                (:id card)
+              :visualization_settings {}}))))
 
 (defn add-text-card
   "Add a text card to dashboard `dashboard` at position [`x`, `y`]."
   [dashboard {:keys [text width height visualization-settings]} [x y]]
   (update dashboard :ordered_cards conj
-          {:creator_id             api/*current-user-id*
-           :visualization_settings (merge
-                                    {:text         text
-                                     :virtual_card {:name                   nil
-                                                    :display                :text
-                                                    :dataset_query          {}
-                                                    :visualization_settings {}}}
-                                    visualization-settings)
-           :col                    y
-           :row                    x
-           :size_x                 width
-           :size_y                 height
-           :card                   nil
-           :id                     (gensym)}))
+          (merge (card-defaults)
+                 {:creator_id             api/*current-user-id*
+                  :visualization_settings (merge
+                                            {:text         text
+                                             :virtual_card {:name                   nil
+                                                            :display                :text
+                                                            :dataset_query          {}
+                                                            :visualization_settings {}}}
+                                            visualization-settings)
+                  :col                    y
+                  :row                    x
+                  :size_x                 width
+                  :size_y                 height
+                  :card                   nil})))
 
 (defn- make-grid
   [width height]

test/metabase/api/automagic_dashboards_test.clj

@@ -15,10 +15,25 @@
    [metabase.transforms.materialize :as tf.materialize]
    [metabase.transforms.specs :as tf.specs]
    [metabase.util :as u]
+   [schema.core :as s]
    [toucan2.tools.with-temp :as t2.with-temp]))
 
 (use-fixtures :once (fixtures/initialize :db :web-server :test-users :test-users-personal-collections))
 
+(defn- ordered-cards-schema-check
+  [ordered_cards]
+  (testing "check if all cards in ordered_cards contain the required fields"
+    (doseq [card ordered_cards]
+      (is (schema= {:id                     (s/cond-pre s/Str s/Int)
+                    :dashboard_tab_id       (s/maybe s/Int)
+                    :row                    s/Int
+                    :col                    s/Int
+                    :size_x                 s/Int
+                    :size_y                 s/Int
+                    :visualization_settings (s/maybe (s/named clojure.lang.IPersistentMap "valid map"))
+                    s/Any                   s/Any}
+                   card)))))
+
 (defn- api-call
   ([template args]
    (api-call template args (constantly true)))
@@ -30,7 +45,9 @@
    (mt/with-test-user :rasta
      (with-dashboard-cleanup
        (let [api-endpoint (apply format (str "automagic-dashboards/" template) args)
-             result       (validation-fn (mt/user-http-request :rasta :get 200 api-endpoint))]
+             resp         (mt/user-http-request :rasta :get 200 api-endpoint)
+             _            (ordered-cards-schema-check (:ordered_cards resp))
+             result       (validation-fn resp)]
          (when (and result
                     (try
                       (testing "Endpoint should return 403 if user does not have permissions"