Merge pull request #9515 from metabase/dashboard-fixes

Dashboard query cancellation + misc

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

@@ -48,7 +48,7 @@ export default class DashCard extends Component {
 
     // HACK: way to scroll to a newly added card
     if (dashcard.justAdded) {
-      ReactDOM.findDOMNode(this).scrollIntoView();
+      ReactDOM.findDOMNode(this).scrollIntoView({ block: "nearest" });
       markNewCardSeen(dashcard.id);
     }
   }

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

@@ -78,6 +78,7 @@ type Props = {
     reload: boolean,
     clear: boolean,
   }) => Promise<void>,
+  cancelFetchDashboardCardData: () => Promise<void>,
 
   setEditingParameter: (parameterId: ?ParameterId) => void,
   setEditingDashboard: (isEditing: boolean) => void,
@@ -123,6 +124,7 @@ type State = {
   error: ?ApiError,
 };
 
+// NOTE: move DashboardControls HoC to container
 @DashboardControls
 export default class Dashboard extends Component {
   props: Props;
@@ -159,6 +161,7 @@ export default class Dashboard extends Component {
     isEditable: true,
   };
 
+  // NOTE: all of these lifecycle methods should be replaced with DashboardData HoC in container
   componentDidMount() {
     this.loadDashboard(this.props.dashboardId);
   }
@@ -174,6 +177,10 @@ export default class Dashboard extends Component {
     }
   }
 
+  componentWillUnmount() {
+    this.props.cancelFetchDashboardCardData();
+  }
+
   async loadDashboard(dashboardId: DashboardId) {
     this.props.initialize();
 

frontend/src/metabase/dashboard/containers/DashboardApp.jsx

@@ -66,6 +66,7 @@ type DashboardAppState = {
 
 @connect(mapStateToProps, mapDispatchToProps)
 @title(({ dashboard }) => dashboard && dashboard.name)
+// NOTE: should use DashboardControls and DashboardData HoCs here?
 export default class DashboardApp extends Component {
   state: DashboardAppState = {
     addCardOnLoad: null,

frontend/src/metabase/dashboard/dashboard.js

@@ -11,6 +11,7 @@ import {
   createThunkAction,
 } from "metabase/lib/redux";
 import { open } from "metabase/lib/dom";
+import { defer } from "metabase/lib/promise";
 import { normalize, schema } from "normalizr";
 
 import Dashboards from "metabase/entities/dashboards";
@@ -93,6 +94,12 @@ export const UPDATE_DASHCARD_ID = "metabase/dashboard/UPDATE_DASHCARD_ID";
 export const FETCH_DASHBOARD_CARD_DATA =
   "metabase/dashboard/FETCH_DASHBOARD_CARD_DATA";
 export const FETCH_CARD_DATA = "metabase/dashboard/FETCH_CARD_DATA";
+
+export const CANCEL_FETCH_DASHBOARD_CARD_DATA =
+  "metabase/dashboard/CANCEL_FETCH_DASHBOARD_CARD_DATA";
+export const CANCEL_FETCH_CARD_DATA =
+  "metabase/dashboard/CANCEL_FETCH_CARD_DATA";
+
 export const MARK_CARD_AS_SLOW = "metabase/dashboard/MARK_CARD_AS_SLOW";
 export const CLEAR_CARD_DATA = "metabase/dashboard/CLEAR_CARD_DATA";
 
@@ -374,25 +381,63 @@ export async function fetchDataOrError(dataPromise) {
   }
 }
 
+function getAllDashboardCards(dashboard) {
+  const results = [];
+  if (dashboard) {
+    for (const dashcard of dashboard.ordered_cards) {
+      const cards = [dashcard.card].concat(dashcard.series || []);
+      results.push(...cards.map(card => ({ card, dashcard })));
+    }
+  }
+  return results;
+}
+
+function isVirtualDashCard(dashcard) {
+  return _.isObject(dashcard.visualization_settings.virtual_card);
+}
+
 export const fetchDashboardCardData = createThunkAction(
   FETCH_DASHBOARD_CARD_DATA,
-  options => async (dispatch, getState) => {
+  options => (dispatch, getState) => {
     const dashboard = getDashboardComplete(getState());
-    if (dashboard) {
-      for (const dashcard of dashboard.ordered_cards) {
-        // we skip over virtual cards, i.e. dashcards that do not have backing cards in the backend
-        if (_.isObject(dashcard.visualization_settings.virtual_card)) {
-          continue;
-        }
-        const cards = [dashcard.card].concat(dashcard.series || []);
-        for (const card of cards) {
-          dispatch(fetchCardData(card, dashcard, options));
-        }
+    for (const { card, dashcard } of getAllDashboardCards(dashboard)) {
+      // we skip over virtual cards, i.e. dashcards that do not have backing cards in the backend
+      if (!isVirtualDashCard(dashcard)) {
+        dispatch(fetchCardData(card, dashcard, options));
       }
     }
   },
 );
 
+export const cancelFetchDashboardCardData = createThunkAction(
+  CANCEL_FETCH_DASHBOARD_CARD_DATA,
+  () => (dispatch, getState) => {
+    const dashboard = getDashboardComplete(getState());
+    for (const { card, dashcard } of getAllDashboardCards(dashboard)) {
+      dispatch(cancelFetchCardData(card.id, dashcard.id));
+    }
+  },
+);
+
+// TODO: this doesn't need to be stored in Redux, does it?
+const cardDataCancelDeferreds = {};
+
+// machinery to support query cancellation
+export const cancelFetchCardData = createAction(
+  CANCEL_FETCH_CARD_DATA,
+  (card_id, dashcard_id) => {
+    const deferred = cardDataCancelDeferreds[`${dashcard_id},${card_id}`];
+    if (deferred) {
+      deferred.resolve();
+      cardDataCancelDeferreds[`${dashcard_id},${card_id}`] = null;
+    }
+    return { dashcard_id, card_id };
+  },
+);
+function setFetchCardDataCancel(card_id, dashcard_id, deferred) {
+  cardDataCancelDeferreds[`${dashcard_id},${card_id}`] = deferred;
+}
+
 export const fetchCardData = createThunkAction(FETCH_CARD_DATA, function(
   card,
   dashcard,
@@ -443,6 +488,8 @@ export const fetchCardData = createThunkAction(FETCH_CARD_DATA, function(
       }
     }
 
+    cancelFetchCardData(card.id, dashcard.id);
+
     if (clear) {
       // clears the card data to indicate the card is reloading
       dispatch(clearCardData(card.id, dashcard.id));
@@ -457,40 +504,65 @@ export const fetchCardData = createThunkAction(FETCH_CARD_DATA, function(
       }
     }, DATASET_SLOW_TIMEOUT);
 
+    const deferred = defer();
+    setFetchCardDataCancel(card.id, dashcard.id, deferred);
+
+    let cancelled = false;
+    deferred.promise.then(() => {
+      cancelled = true;
+    });
+
+    const queryOptions = {
+      cancelled: deferred.promise,
+    };
+
     // make the actual request
     if (dashboardType === "public") {
       result = await fetchDataOrError(
-        PublicApi.dashboardCardQuery({
-          uuid: dashcard.dashboard_id,
-          cardId: card.id,
-          parameters: datasetQuery.parameters
-            ? JSON.stringify(datasetQuery.parameters)
-            : undefined,
-        }),
+        PublicApi.dashboardCardQuery(
+          {
+            uuid: dashcard.dashboard_id,
+            cardId: card.id,
+            parameters: datasetQuery.parameters
+              ? JSON.stringify(datasetQuery.parameters)
+              : undefined,
+          },
+          queryOptions,
+        ),
       );
     } else if (dashboardType === "embed") {
       result = await fetchDataOrError(
-        EmbedApi.dashboardCardQuery({
-          token: dashcard.dashboard_id,
-          dashcardId: dashcard.id,
-          cardId: card.id,
-          ...getParametersBySlug(dashboard.parameters, parameterValues),
-        }),
+        EmbedApi.dashboardCardQuery(
+          {
+            token: dashcard.dashboard_id,
+            dashcardId: dashcard.id,
+            cardId: card.id,
+            ...getParametersBySlug(dashboard.parameters, parameterValues),
+          },
+          queryOptions,
+        ),
       );
     } else if (dashboardType === "transient") {
-      result = await fetchDataOrError(MetabaseApi.dataset(datasetQuery));
+      result = await fetchDataOrError(
+        MetabaseApi.dataset(datasetQuery),
+        queryOptions,
+      );
     } else {
       result = await fetchDataOrError(
-        CardApi.query({ cardId: card.id, parameters: datasetQuery.parameters }),
+        CardApi.query(
+          { cardId: card.id, parameters: datasetQuery.parameters },
+          queryOptions,
+        ),
       );
     }
 
+    setFetchCardDataCancel(card.id, dashcard.id, null);
     clearTimeout(slowCardTimer);
 
     return {
       dashcard_id: dashcard.id,
       card_id: card.id,
-      result: result,
+      result: cancelled ? null : result,
     };
   };
 });

frontend/src/metabase/dashboard/hoc/DashboardData.jsx

@@ -58,6 +58,7 @@ type Props = {
     reload: boolean,
     clear: boolean,
   }) => Promise<void>,
+  cancelFetchDashboardCardData: () => Promise<void>,
   setParameterValue: (id: string, value: string) => void,
   setErrorPage: (error: { status: number }) => void,
 };
@@ -91,6 +92,10 @@ export default (ComposedComponent: ReactClass<any>) =>
         this.load(this.props);
       }
 
+      componentWillUnmount() {
+        this.props.cancelFetchDashboardCardData();
+      }
+
       componentWillReceiveProps(nextProps: Props) {
         if (nextProps.dashboardId !== this.props.dashboardId) {
           this.load(nextProps);

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

@@ -75,12 +75,14 @@ type Props = {
     reload: boolean,
     clear: boolean,
   }) => Promise<void>,
+  cancelFetchDashboardCardData: () => Promise<void>,
   setParameterValue: (id: string, value: string) => void,
   setErrorPage: (error: { status: number }) => void,
 };
 
 @connect(mapStateToProps, mapDispatchToProps)
 @DashboardControls
+// NOTE: this should use DashboardData HoC
 export default class PublicDashboard extends Component {
   props: Props;
 
@@ -112,6 +114,10 @@ export default class PublicDashboard extends Component {
     }
   }
 
+  componentWillUnmount() {
+    this.props.cancelFetchDashboardCardData();
+  }
+
   componentWillReceiveProps(nextProps: Props) {
     if (!_.isEqual(this.props.parameterValues, nextProps.parameterValues)) {
       this.props.fetchDashboardCardData({ reload: false, clear: true });

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

@@ -136,6 +136,8 @@ export default class PublicQuestion extends Component {
     const parameters = getParameters(card);
 
     try {
+      this.setState({ result: null });
+
       let newResult;
       if (token) {
         // embeds apply parameter values server-side