Fixes several bugs on dashboard page (see description) (#4571)

* Move each hook to own file; move hooks and components to own folders

* Update URL and timer only when refresh rate changes

* Skip dashboard refresh if previous refresh is still running

* Fix test

client/app/pages/dashboards/DashboardList.jsx

@@ -17,7 +17,7 @@ import Layout from "@/components/layouts/ContentWithSidebar";
 
 import { Dashboard } from "@/services/dashboard";
 
-import DashboardListEmptyState from "./DashboardListEmptyState";
+import DashboardListEmptyState from "./components/DashboardListEmptyState";
 
 import "./dashboard-list.css";
 

client/app/pages/dashboards/DashboardPage.jsx

@@ -23,7 +23,7 @@ import getTags from "@/services/getTags";
 import { clientConfig } from "@/services/auth";
 import { policy } from "@/services/policy";
 import { durationHumanize } from "@/lib/utils";
-import useDashboard, { DashboardStatusEnum } from "./useDashboard";
+import useDashboard, { DashboardStatusEnum } from "./hooks/useDashboard";
 
 import "./DashboardPage.less";
 

client/app/pages/dashboards/PublicDashboardPage.jsx

@@ -10,7 +10,7 @@ import Filters from "@/components/Filters";
 import { ErrorBoundaryContext } from "@/components/ErrorBoundary";
 import { Dashboard } from "@/services/dashboard";
 import logoUrl from "@/assets/images/redash_icon_small.png";
-import useDashboard from "./useDashboard";
+import useDashboard from "./hooks/useDashboard";
 
 import "./PublicDashboardPage.less";
 

client/app/pages/dashboards/hooks/useDashboard.js

@@ -1,40 +1,20 @@
 import { useState, useEffect, useMemo, useCallback } from "react";
-import {
-  isEmpty,
-  isNaN,
-  includes,
-  compact,
-  map,
-  has,
-  pick,
-  keys,
-  extend,
-  every,
-  find,
-  debounce,
-  isMatch,
-  pickBy,
-  max,
-  min,
-  get,
-} from "lodash";
+import { isEmpty, includes, compact, map, has, pick, keys, extend, every, get } from "lodash";
 import notification from "@/services/notification";
 import location from "@/services/location";
 import { Dashboard, collectDashboardFilters } from "@/services/dashboard";
 import { currentUser } from "@/services/auth";
 import recordEvent from "@/services/recordEvent";
-import { policy } from "@/services/policy";
 import AddWidgetDialog from "@/components/dashboards/AddWidgetDialog";
 import TextboxDialog from "@/components/dashboards/TextboxDialog";
 import PermissionsEditorDialog from "@/components/PermissionsEditorDialog";
 import { editableMappingsToParameterMappings, synchronizeWidgetTitles } from "@/components/ParameterMappingInput";
-import ShareDashboardDialog from "./ShareDashboardDialog";
+import ShareDashboardDialog from "../components/ShareDashboardDialog";
+import useFullscreenHandler from "./useFullscreenHandler";
+import useRefreshRateHandler from "./useRefreshRateHandler";
+import useEditModeHandler from "./useEditModeHandler";
 
-export const DashboardStatusEnum = {
-  SAVED: "saved",
-  SAVING: "saving",
-  SAVING_FAILED: "saving_failed",
-};
+export { DashboardStatusEnum } from "./useEditModeHandler";
 
 function getAffectedWidgets(widgets, updatedParameters = []) {
   return !isEmpty(updatedParameters)
@@ -51,133 +31,6 @@ function getAffectedWidgets(widgets, updatedParameters = []) {
     : widgets;
 }
 
-function getChangedPositions(widgets, nextPositions = {}) {
-  return pickBy(nextPositions, (nextPos, widgetId) => {
-    const widget = find(widgets, { id: Number(widgetId) });
-    const prevPos = widget.options.position;
-    return !isMatch(prevPos, nextPos);
-  });
-}
-
-function getLimitedRefreshRate(refreshRate) {
-  const allowedIntervals = policy.getDashboardRefreshIntervals();
-  return max([30, min(allowedIntervals), refreshRate]);
-}
-
-function getRefreshRateFromUrl() {
-  const refreshRate = parseFloat(location.search.refresh);
-  return isNaN(refreshRate) ? null : getLimitedRefreshRate(refreshRate);
-}
-
-function useFullscreenHandler() {
-  const [fullscreen, setFullscreen] = useState(has(location.search, "fullscreen"));
-  useEffect(() => {
-    document.body.classList.toggle("headless", fullscreen);
-    location.setSearch({ fullscreen: fullscreen ? true : null }, true);
-  }, [fullscreen]);
-
-  const toggleFullscreen = () => setFullscreen(!fullscreen);
-  return [fullscreen, toggleFullscreen];
-}
-
-function useRefreshRateHandler(refreshDashboard) {
-  const [refreshRate, setRefreshRate] = useState(getRefreshRateFromUrl());
-
-  useEffect(() => {
-    location.setSearch({ refresh: refreshRate || null }, true);
-    if (refreshRate) {
-      const refreshTimer = setInterval(refreshDashboard, refreshRate * 1000);
-      return () => clearInterval(refreshTimer);
-    }
-  }, [refreshDashboard, refreshRate]);
-
-  return [refreshRate, rate => setRefreshRate(getLimitedRefreshRate(rate)), () => setRefreshRate(null)];
-}
-
-function useEditModeHandler(canEditDashboard, widgets) {
-  const [editingLayout, setEditingLayout] = useState(canEditDashboard && has(location.search, "edit"));
-  const [dashboardStatus, setDashboardStatus] = useState(DashboardStatusEnum.SAVED);
-  const [recentPositions, setRecentPositions] = useState([]);
-  const [doneBtnClickedWhileSaving, setDoneBtnClickedWhileSaving] = useState(false);
-
-  useEffect(() => {
-    location.setSearch({ edit: editingLayout ? true : null }, true);
-  }, [editingLayout]);
-
-  useEffect(() => {
-    if (doneBtnClickedWhileSaving && dashboardStatus === DashboardStatusEnum.SAVED) {
-      setDoneBtnClickedWhileSaving(false);
-      setEditingLayout(false);
-    }
-  }, [doneBtnClickedWhileSaving, dashboardStatus]);
-
-  const saveDashboardLayout = useCallback(
-    positions => {
-      if (!canEditDashboard) {
-        setDashboardStatus(DashboardStatusEnum.SAVED);
-        return;
-      }
-
-      const changedPositions = getChangedPositions(widgets, positions);
-
-      setDashboardStatus(DashboardStatusEnum.SAVING);
-      setRecentPositions(positions);
-      const saveChangedWidgets = map(changedPositions, (position, id) => {
-        // find widget
-        const widget = find(widgets, { id: Number(id) });
-
-        // skip already deleted widget
-        if (!widget) {
-          return Promise.resolve();
-        }
-
-        return widget.save("options", { position });
-      });
-
-      return Promise.all(saveChangedWidgets)
-        .then(() => setDashboardStatus(DashboardStatusEnum.SAVED))
-        .catch(() => {
-          setDashboardStatus(DashboardStatusEnum.SAVING_FAILED);
-          notification.error("Error saving changes.");
-        });
-    },
-    [canEditDashboard, widgets]
-  );
-
-  const saveDashboardLayoutDebounced = useCallback(
-    (...args) => {
-      setDashboardStatus(DashboardStatusEnum.SAVING);
-      return debounce(() => saveDashboardLayout(...args), 2000)();
-    },
-    [saveDashboardLayout]
-  );
-
-  const retrySaveDashboardLayout = useCallback(() => saveDashboardLayout(recentPositions), [
-    recentPositions,
-    saveDashboardLayout,
-  ]);
-
-  const setEditing = useCallback(
-    editing => {
-      if (!editing && dashboardStatus !== DashboardStatusEnum.SAVED) {
-        setDoneBtnClickedWhileSaving(true);
-        return;
-      }
-      setEditingLayout(canEditDashboard && editing);
-    },
-    [dashboardStatus, canEditDashboard]
-  );
-
-  return {
-    editingLayout: canEditDashboard && editingLayout,
-    setEditingLayout: setEditing,
-    saveDashboardLayout: editingLayout ? saveDashboardLayoutDebounced : saveDashboardLayout,
-    retrySaveDashboardLayout,
-    doneBtnClickedWhileSaving,
-    dashboardStatus,
-  };
-}
-
 function useDashboard(dashboardData) {
   const [dashboard, setDashboard] = useState(dashboardData);
   const [filters, setFilters] = useState([]);
@@ -272,10 +125,12 @@ function useDashboard(dashboardData) {
 
   const refreshDashboard = useCallback(
     updatedParameters => {
-      setRefreshing(true);
-      loadDashboard(true, updatedParameters).finally(() => setRefreshing(false));
+      if (!refreshing) {
+        setRefreshing(true);
+        loadDashboard(true, updatedParameters).finally(() => setRefreshing(false));
+      }
     },
-    [loadDashboard]
+    [refreshing, loadDashboard]
   );
 
   const archiveDashboard = useCallback(() => {

client/app/pages/dashboards/hooks/useEditModeHandler.js

@@ -0,0 +1,102 @@
+import { debounce, find, has, isMatch, map, pickBy } from "lodash";
+import { useCallback, useEffect, useState } from "react";
+import location from "@/services/location";
+import notification from "@/services/notification";
+
+export const DashboardStatusEnum = {
+  SAVED: "saved",
+  SAVING: "saving",
+  SAVING_FAILED: "saving_failed",
+};
+
+function getChangedPositions(widgets, nextPositions = {}) {
+  return pickBy(nextPositions, (nextPos, widgetId) => {
+    const widget = find(widgets, { id: Number(widgetId) });
+    const prevPos = widget.options.position;
+    return !isMatch(prevPos, nextPos);
+  });
+}
+
+export default function useEditModeHandler(canEditDashboard, widgets) {
+  const [editingLayout, setEditingLayout] = useState(canEditDashboard && has(location.search, "edit"));
+  const [dashboardStatus, setDashboardStatus] = useState(DashboardStatusEnum.SAVED);
+  const [recentPositions, setRecentPositions] = useState([]);
+  const [doneBtnClickedWhileSaving, setDoneBtnClickedWhileSaving] = useState(false);
+
+  useEffect(() => {
+    location.setSearch({ edit: editingLayout ? true : null }, true);
+  }, [editingLayout]);
+
+  useEffect(() => {
+    if (doneBtnClickedWhileSaving && dashboardStatus === DashboardStatusEnum.SAVED) {
+      setDoneBtnClickedWhileSaving(false);
+      setEditingLayout(false);
+    }
+  }, [doneBtnClickedWhileSaving, dashboardStatus]);
+
+  const saveDashboardLayout = useCallback(
+    positions => {
+      if (!canEditDashboard) {
+        setDashboardStatus(DashboardStatusEnum.SAVED);
+        return;
+      }
+
+      const changedPositions = getChangedPositions(widgets, positions);
+
+      setDashboardStatus(DashboardStatusEnum.SAVING);
+      setRecentPositions(positions);
+      const saveChangedWidgets = map(changedPositions, (position, id) => {
+        // find widget
+        const widget = find(widgets, { id: Number(id) });
+
+        // skip already deleted widget
+        if (!widget) {
+          return Promise.resolve();
+        }
+
+        return widget.save("options", { position });
+      });
+
+      return Promise.all(saveChangedWidgets)
+        .then(() => setDashboardStatus(DashboardStatusEnum.SAVED))
+        .catch(() => {
+          setDashboardStatus(DashboardStatusEnum.SAVING_FAILED);
+          notification.error("Error saving changes.");
+        });
+    },
+    [canEditDashboard, widgets]
+  );
+
+  const saveDashboardLayoutDebounced = useCallback(
+    (...args) => {
+      setDashboardStatus(DashboardStatusEnum.SAVING);
+      return debounce(() => saveDashboardLayout(...args), 2000)();
+    },
+    [saveDashboardLayout]
+  );
+
+  const retrySaveDashboardLayout = useCallback(() => saveDashboardLayout(recentPositions), [
+    recentPositions,
+    saveDashboardLayout,
+  ]);
+
+  const setEditing = useCallback(
+    editing => {
+      if (!editing && dashboardStatus !== DashboardStatusEnum.SAVED) {
+        setDoneBtnClickedWhileSaving(true);
+        return;
+      }
+      setEditingLayout(canEditDashboard && editing);
+    },
+    [dashboardStatus, canEditDashboard]
+  );
+
+  return {
+    editingLayout: canEditDashboard && editingLayout,
+    setEditingLayout: setEditing,
+    saveDashboardLayout: editingLayout ? saveDashboardLayoutDebounced : saveDashboardLayout,
+    retrySaveDashboardLayout,
+    doneBtnClickedWhileSaving,
+    dashboardStatus,
+  };
+}

client/app/pages/dashboards/hooks/useFullscreenHandler.js

@@ -0,0 +1,14 @@
+import { has } from "lodash";
+import { useEffect, useState } from "react";
+import location from "@/services/location";
+
+export default function useFullscreenHandler() {
+  const [fullscreen, setFullscreen] = useState(has(location.search, "fullscreen"));
+  useEffect(() => {
+    document.body.classList.toggle("headless", fullscreen);
+    location.setSearch({ fullscreen: fullscreen ? true : null }, true);
+  }, [fullscreen]);
+
+  const toggleFullscreen = () => setFullscreen(!fullscreen);
+  return [fullscreen, toggleFullscreen];
+}

client/app/pages/dashboards/hooks/useRefreshRateHandler.js

@@ -0,0 +1,40 @@
+import { isNaN, max, min } from "lodash";
+import { useEffect, useState, useRef, useMemo } from "react";
+import location from "@/services/location";
+import { policy } from "@/services/policy";
+
+function getLimitedRefreshRate(refreshRate) {
+  const allowedIntervals = policy.getDashboardRefreshIntervals();
+  return max([30, min(allowedIntervals), refreshRate]);
+}
+
+function getRefreshRateFromUrl() {
+  const refreshRate = parseFloat(location.search.refresh);
+  return isNaN(refreshRate) ? null : getLimitedRefreshRate(refreshRate);
+}
+
+export default function useRefreshRateHandler(refreshDashboard) {
+  const [refreshRate, setRefreshRate] = useState(getRefreshRateFromUrl());
+
+  // `refreshDashboard` may change quite frequently (on every update of `dashboard` instance), but we
+  // have to keep the same timer running, because timer will restart when re-creating, and instead of
+  // running refresh every N seconds - it will run refresh every N seconds after last dashboard update
+  // (which is not right obviously)
+  const refreshDashboardRef = useRef();
+  refreshDashboardRef.current = refreshDashboard;
+
+  // URL and timer should be updated only when `refreshRate` changes
+  useEffect(() => {
+    location.setSearch({ refresh: refreshRate || null }, true);
+    if (refreshRate) {
+      const refreshTimer = setInterval(() => {
+        refreshDashboardRef.current();
+      }, refreshRate * 1000);
+      return () => clearInterval(refreshTimer);
+    }
+  }, [refreshRate]);
+
+  return useMemo(() => [refreshRate, rate => setRefreshRate(getLimitedRefreshRate(rate)), () => setRefreshRate(null)], [
+    refreshRate,
+  ]);
+}

client/app/pages/queries/components/QueryExecutionStatus.jsx

@@ -37,6 +37,7 @@ export default function QueryExecutionStatus({ status, updatedAt, error, isCance
 
   return (
     <Alert
+      data-test="QueryExecutionStatus"
       type={alertType}
       message={
         <div className="d-flex align-items-center">

client/cypress/integration/query/parameter_spec.js

@@ -117,7 +117,7 @@ describe("Parameter", () => {
 
       cy.getByTestId("ParameterApplyButton").click();
       // ensure that query is being executed
-      cy.get('[data-test="ExecuteButton"]:disabled').should("exist");
+      cy.getByTestId("QueryExecutionStatus").should("exist");
 
       cy.getByTestId("TableVisualization").should("contain", "value2");
     });