From 984fbac1adeb6746e04814c9957c36eac0e1f8fa Mon Sep 17 00:00:00 2001
From: Bogdan Matei <bogdan.matei@grafana.com>
Date: Mon, 18 Nov 2024 17:09:03 +0200
Subject: [PATCH] Dashboard: Fix dashboard reload behavior (#96427)

---
 .../pages/DashboardScenePageStateManager.ts   |  42 ++++--
 .../scene/DashboardReloadBehavior.ts          |  54 ++++----
 .../scopes/tests/dashboardReload.test.ts      | 123 ++++++++----------
 3 files changed, 116 insertions(+), 103 deletions(-)

diff --git a/public/app/features/dashboard-scene/pages/DashboardScenePageStateManager.ts b/public/app/features/dashboard-scene/pages/DashboardScenePageStateManager.ts
index 7c8d9399940..d70663f1491 100644
--- a/public/app/features/dashboard-scene/pages/DashboardScenePageStateManager.ts
+++ b/public/app/features/dashboard-scene/pages/DashboardScenePageStateManager.ts
@@ -44,7 +44,15 @@ export interface LoadDashboardOptions {
   uid: string;
   route: DashboardRoutes;
   urlFolderUid?: string;
-  queryParams?: UrlQueryMap;
+  params?: {
+    version: number;
+    scopes: string[];
+    timeRange: {
+      from: string;
+      to: string;
+    };
+    variables: UrlQueryMap;
+  };
 }
 
 export class DashboardScenePageStateManager extends StateManagerBase<DashboardScenePageState> {
@@ -59,11 +67,11 @@ export class DashboardScenePageStateManager extends StateManagerBase<DashboardSc
     uid,
     route,
     urlFolderUid,
-    queryParams,
+    params,
   }: LoadDashboardOptions): Promise<DashboardDTO | null> {
     const cacheKey = route === DashboardRoutes.Home ? HOME_DASHBOARD_CACHE_KEY : uid;
 
-    if (!queryParams) {
+    if (!params) {
       const cachedDashboard = this.getDashboardFromCache(cacheKey);
 
       if (cachedDashboard) {
@@ -97,6 +105,15 @@ export class DashboardScenePageStateManager extends StateManagerBase<DashboardSc
           return await dashboardLoaderSrv.loadDashboard('public', '', uid);
         }
         default:
+          const queryParams = params
+            ? {
+                version: params.version,
+                scopes: params.scopes,
+                from: params.timeRange.from,
+                to: params.timeRange.to,
+                ...params.variables,
+              }
+            : undefined;
           rsp = await dashboardLoaderSrv.loadDashboard('db', '', uid, queryParams);
 
           if (route === DashboardRoutes.Embedded) {
@@ -188,17 +205,26 @@ export class DashboardScenePageStateManager extends StateManagerBase<DashboardSc
     }
   }
 
-  public async reloadDashboard(queryParams?: LoadDashboardOptions['queryParams'] | undefined) {
-    if (!this.state.options) {
+  public async reloadDashboard(params: LoadDashboardOptions['params']) {
+    const stateOptions = this.state.options;
+
+    if (!stateOptions) {
       return;
     }
 
     const options = {
-      ...this.state.options,
-      queryParams,
+      ...stateOptions,
+      params,
     };
 
-    if (isEqual(options, this.state.options)) {
+    // We shouldn't check all params since:
+    // - version doesn't impact the new dashboard, and it's there for increased compatibility
+    // - time range is almost always different for relative time ranges and absolute time ranges do not trigger subsequent reloads
+    // - other params don't affect the dashboard content
+    if (
+      isEqual(options.params?.variables, stateOptions.params?.variables) &&
+      isEqual(options.params?.scopes, stateOptions.params?.scopes)
+    ) {
       return;
     }
 
diff --git a/public/app/features/dashboard-scene/scene/DashboardReloadBehavior.ts b/public/app/features/dashboard-scene/scene/DashboardReloadBehavior.ts
index 443694615c1..71b13b7ecd0 100644
--- a/public/app/features/dashboard-scene/scene/DashboardReloadBehavior.ts
+++ b/public/app/features/dashboard-scene/scene/DashboardReloadBehavior.ts
@@ -1,8 +1,8 @@
-import { isEqual } from 'lodash';
+import { debounce, isEqual } from 'lodash';
 
 import { UrlQueryMap } from '@grafana/data';
 import { sceneGraph, SceneObjectBase, SceneObjectState, VariableDependencyConfig } from '@grafana/scenes';
-import { getClosestScopesFacade, ScopesFacade } from 'app/features/scopes';
+import { getClosestScopesFacade } from 'app/features/scopes';
 
 import { getDashboardScenePageStateManager } from '../pages/DashboardScenePageStateManager';
 
@@ -13,27 +13,25 @@ export interface DashboardReloadBehaviorState extends SceneObjectState {
 }
 
 export class DashboardReloadBehavior extends SceneObjectBase<DashboardReloadBehaviorState> {
-  private _scopesFacade: ScopesFacade | null = null;
-
   constructor(state: DashboardReloadBehaviorState) {
     const shouldReload = state.reloadOnParamsChange && state.uid;
 
     super(state);
 
-    this.reloadDashboard = this.reloadDashboard.bind(this);
+    // Sometimes the reload is triggered multiple subsequent times
+    // Debouncing it prevents double/triple reloads
+    this.reloadDashboard = debounce(this.reloadDashboard).bind(this);
 
     if (shouldReload) {
       this.addActivationHandler(() => {
-        this._scopesFacade = getClosestScopesFacade(this);
+        getClosestScopesFacade(this)?.setState({
+          handler: this.reloadDashboard,
+        });
 
         this._variableDependency = new VariableDependencyConfig(this, {
           onAnyVariableChanged: this.reloadDashboard,
         });
 
-        this._scopesFacade?.setState({
-          handler: this.reloadDashboard,
-        });
-
         this._subs.add(
           sceneGraph.getTimeRange(this).subscribeToState((newState, prevState) => {
             if (!isEqual(newState.value, prevState.value)) {
@@ -41,6 +39,8 @@ export class DashboardReloadBehavior extends SceneObjectBase<DashboardReloadBeha
             }
           })
         );
+
+        this.reloadDashboard();
       });
     }
   }
@@ -59,21 +59,25 @@ export class DashboardReloadBehavior extends SceneObjectBase<DashboardReloadBeha
     if (!this.isEditing() && !this.isWaitingForVariables()) {
       const timeRange = sceneGraph.getTimeRange(this);
 
-      let params: UrlQueryMap = {
-        version: this.state.version,
-        scopes: this._scopesFacade?.value.map((scope) => scope.metadata.name),
-        ...timeRange.urlSync?.getUrlState(),
-      };
-
-      params = sceneGraph.getVariables(this).state.variables.reduce<UrlQueryMap>(
-        (acc, variable) => ({
-          ...acc,
-          ...variable.urlSync?.getUrlState(),
-        }),
-        params
-      );
-
-      getDashboardScenePageStateManager().reloadDashboard(params);
+      // This is wrapped in setTimeout in order to allow variables and scopes to be set in the URL before actually reloading the dashboard
+      setTimeout(() => {
+        getDashboardScenePageStateManager().reloadDashboard({
+          version: this.state.version!,
+          scopes: getClosestScopesFacade(this)?.value.map((scope) => scope.metadata.name) ?? [],
+          // We're not using the getUrlState from timeRange since it makes more sense to pass the absolute timestamps as opposed to relative time
+          timeRange: {
+            from: timeRange.state.value.from.toISOString(),
+            to: timeRange.state.value.to.toISOString(),
+          },
+          variables: sceneGraph.getVariables(this).state.variables.reduce<UrlQueryMap>(
+            (acc, variable) => ({
+              ...acc,
+              ...variable.urlSync?.getUrlState(),
+            }),
+            {}
+          ),
+        });
+      });
     }
   }
 }
diff --git a/public/app/features/scopes/tests/dashboardReload.test.ts b/public/app/features/scopes/tests/dashboardReload.test.ts
index 84856e82aec..24b07bca51a 100644
--- a/public/app/features/scopes/tests/dashboardReload.test.ts
+++ b/public/app/features/scopes/tests/dashboardReload.test.ts
@@ -1,7 +1,8 @@
 import { config } from '@grafana/runtime';
 import { setDashboardAPI } from 'app/features/dashboard/api/dashboard_api';
+import { getDashboardScenePageStateManager } from 'app/features/dashboard-scene/pages/DashboardScenePageStateManager';
 
-import { enterEditMode, updateMyVar, updateScopes, updateTimeRange } from './utils/actions';
+import { clearMocks, enterEditMode, updateMyVar, updateScopes, updateTimeRange } from './utils/actions';
 import { expectDashboardReload, expectNotDashboardReload } from './utils/assertions';
 import { getDatasource, getInstanceSettings, getMock } from './utils/mocks';
 import { renderDashboard, resetScenes } from './utils/render';
@@ -15,84 +16,66 @@ jest.mock('@grafana/runtime', () => ({
   usePluginLinks: jest.fn().mockReturnValue({ links: [] }),
 }));
 
-const runTest = async (
-  reloadDashboardsOnParamsChange: boolean,
-  reloadOnParamsChange: boolean,
-  withUid: boolean,
-  editMode: boolean
-) => {
-  config.featureToggles.reloadDashboardsOnParamsChange = reloadDashboardsOnParamsChange;
-  setDashboardAPI(undefined);
-  const uid = 'dash-1';
-  const dashboardScene = renderDashboard({ uid: withUid ? uid : undefined }, { reloadOnParamsChange });
-
-  if (editMode) {
-    await enterEditMode(dashboardScene);
-  }
-
-  const shouldReload = reloadDashboardsOnParamsChange && reloadOnParamsChange && withUid && !editMode;
-
-  await updateTimeRange(dashboardScene);
-  if (!shouldReload) {
-    expectNotDashboardReload();
-  } else {
-    expectDashboardReload();
-  }
-
-  await updateMyVar(dashboardScene, '2');
-  if (!shouldReload) {
-    expectNotDashboardReload();
-  } else {
-    expectDashboardReload();
-  }
-
-  await updateScopes(['grafana']);
-  if (!shouldReload) {
-    expectNotDashboardReload();
-  } else {
-    expectDashboardReload();
-  }
-};
-
 describe('Dashboard reload', () => {
   beforeAll(() => {
     config.featureToggles.scopeFilters = true;
     config.featureToggles.groupByVariable = true;
   });
 
-  afterEach(async () => {
-    setDashboardAPI(undefined);
-    await resetScenes();
-  });
+  it.each([
+    [false, false, false, false],
+    [false, false, true, false],
+    [false, true, false, false],
+    [false, true, true, false],
+    [true, false, false, false],
+    [true, false, true, false],
+    [true, true, false, true],
+    [true, true, true, true],
+    [true, true, false, false],
+    [true, true, true, false],
+  ])(
+    `reloadDashboardsOnParamsChange: %s, reloadOnParamsChange: %s, withUid: %s, editMode: %s`,
+    async (reloadDashboardsOnParamsChange, reloadOnParamsChange, withUid, editMode) => {
+      config.featureToggles.reloadDashboardsOnParamsChange = reloadDashboardsOnParamsChange;
+      setDashboardAPI(undefined);
 
-  describe('reloadDashboardsOnParamsChange off', () => {
-    describe('reloadOnParamsChange off', () => {
-      it('with UID - no reload', () => runTest(false, false, true, false));
-      it('without UID - no reload', () => runTest(false, false, false, false));
-    });
+      const dashboardScene = renderDashboard({ uid: withUid ? 'dash-1' : undefined }, { reloadOnParamsChange });
 
-    describe('reloadOnParamsChange on', () => {
-      it('with UID - no reload', () => runTest(false, true, true, false));
-      it('without UID - no reload', () => runTest(false, true, false, false));
-    });
-  });
+      if (editMode) {
+        await enterEditMode(dashboardScene);
+      }
 
-  describe('reloadDashboardsOnParamsChange on', () => {
-    describe('reloadOnParamsChange off', () => {
-      it('with UID - no reload', () => runTest(true, false, true, false));
-      it('without UID - no reload', () => runTest(true, false, false, false));
-    });
+      const shouldReload = reloadDashboardsOnParamsChange && reloadOnParamsChange && withUid && !editMode;
 
-    describe('reloadOnParamsChange on', () => {
-      describe('edit mode on', () => {
-        it('with UID - no reload', () => runTest(true, true, true, true));
-        it('without UID - no reload', () => runTest(true, true, false, true));
-      });
+      await updateTimeRange(dashboardScene);
+      await jest.advanceTimersToNextTimerAsync();
+      if (!shouldReload) {
+        expectNotDashboardReload();
+      } else {
+        expectDashboardReload();
+      }
 
-      describe('edit mode off', () => {
-        it('with UID - reload', () => runTest(true, true, true, false));
-        it('without UID - no reload', () => runTest(true, true, false, false));
-      });
-    });
-  });
+      await updateMyVar(dashboardScene, '2');
+      await jest.advanceTimersToNextTimerAsync();
+      if (!shouldReload) {
+        expectNotDashboardReload();
+      } else {
+        expectDashboardReload();
+      }
+
+      await updateScopes(['grafana']);
+      await jest.advanceTimersToNextTimerAsync();
+      if (!shouldReload) {
+        expectNotDashboardReload();
+      } else {
+        expectDashboardReload();
+      }
+
+      getDashboardScenePageStateManager().clearDashboardCache();
+      getDashboardScenePageStateManager().clearSceneCache();
+      setDashboardAPI(undefined);
+      await resetScenes();
+      clearMocks();
+    }
+  );
 });