[v11.0.x] DataTrails: Exploring alternatives to history issues (#87003)

DataTrails: Exploring alternatives to history issues (#86843)

* DataTrails: Exploring alternatives to history issues

* incorporated unit tests from #86817 and #86741

---------

Co-authored-by: Darren Janeczek <darren.janeczek@grafana.com>
(cherry picked from commit c965c27994)

Co-authored-by: Torkel Ödegaard <torkel@grafana.com>

public/app/features/trails/DataTrail.test.tsx

@@ -1,4 +1,5 @@
 import { locationService, setDataSourceSrv } from '@grafana/runtime';
+import { AdHocFiltersVariable, sceneGraph } from '@grafana/scenes';
 
 import { MockDataSourceSrv, mockDataSource } from '../alerting/unified/mocks';
 import { DataSourceType } from '../alerting/unified/utils/datasource';
@@ -7,7 +8,7 @@ import { activateFullSceneTree } from '../dashboard-scene/utils/test-utils';
 import { DataTrail } from './DataTrail';
 import { MetricScene } from './MetricScene';
 import { MetricSelectScene } from './MetricSelectScene';
-import { MetricSelectedEvent } from './shared';
+import { MetricSelectedEvent, VAR_FILTERS } from './shared';
 
 describe('DataTrail', () => {
   beforeAll(() => {
@@ -61,10 +62,6 @@ describe('DataTrail', () => {
         expect(trail.state.history.state.steps[1].type).toBe('metric');
       });
 
-      it('Should set history current step to 1', () => {
-        expect(trail.state.history.state.currentStep).toBe(1);
-      });
-
       it('Should set history currentStep to 1', () => {
         expect(trail.state.history.state.currentStep).toBe(1);
       });
@@ -73,6 +70,10 @@ describe('DataTrail', () => {
         expect(trail.state.history.state.steps[1].parentIndex).toBe(0);
       });
 
+      it('Should have time range `from` be default "now-6h"', () => {
+        expect(trail.state.$timeRange?.state.from).toBe('now-6h');
+      });
+
       describe('And browser back button is pressed', () => {
         locationService.getHistory().goBack();
 
@@ -81,6 +82,271 @@ describe('DataTrail', () => {
           expect(pathname).toEqual(preTrailUrl);
         });
       });
+
+      describe('And when changing the time range `from` to "now-1h"', () => {
+        beforeEach(() => {
+          trail.state.$timeRange?.setState({ from: 'now-1h' });
+        });
+
+        it('should sync state with url', () => {
+          expect(locationService.getSearchObject().from).toBe('now-1h');
+        });
+
+        it('should add history step', () => {
+          expect(trail.state.history.state.steps[2].type).toBe('time');
+        });
+
+        it('Should set history currentStep to 2', () => {
+          expect(trail.state.history.state.currentStep).toBe(2);
+        });
+
+        it('Should set history step 2 parentIndex to 1', () => {
+          expect(trail.state.history.state.steps[2].parentIndex).toBe(1);
+        });
+
+        it('Should have time range `from` be updated "now-1h"', () => {
+          expect(trail.state.$timeRange?.state.from).toBe('now-1h');
+        });
+
+        it('Previous history step should have previous default `from` of "now-6h"', () => {
+          expect(trail.state.history.state.steps[1].trailState.$timeRange?.state.from).toBe('now-6h');
+        });
+
+        it('Current history step should have new `from` of "now-1h"', () => {
+          expect(trail.state.history.state.steps[2].trailState.$timeRange?.state.from).toBe('now-1h');
+        });
+
+        describe('And when traversing back to step 1', () => {
+          beforeEach(() => {
+            trail.state.history.goBackToStep(1);
+          });
+
+          it('Should set history currentStep to 1', () => {
+            expect(trail.state.history.state.currentStep).toBe(1);
+          });
+
+          it('should sync state with url', () => {
+            expect(locationService.getSearchObject().from).toBe('now-6h');
+          });
+
+          it('Should have time range `from` be set back to "now-6h"', () => {
+            expect(trail.state.$timeRange?.state.from).toBe('now-6h');
+          });
+
+          describe('And then when changing the time range `from` to "now-15m"', () => {
+            beforeEach(() => {
+              trail.state.$timeRange?.setState({ from: 'now-15m' });
+            });
+
+            it('should sync state with url', () => {
+              expect(locationService.getSearchObject().from).toBe('now-15m');
+            });
+
+            it('should add history step', () => {
+              expect(trail.state.history.state.steps[3].type).toBe('time');
+            });
+
+            it('Should set history currentStep to 3', () => {
+              expect(trail.state.history.state.currentStep).toBe(3);
+            });
+
+            it('Should set history step 3 parentIndex to 1', () => {
+              expect(trail.state.history.state.steps[3].parentIndex).toBe(1);
+            });
+
+            it('Should have time range `from` be updated "now-15m"', () => {
+              expect(trail.state.$timeRange?.state.from).toBe('now-15m');
+            });
+
+            it('History step 1 (parent) should have previous default `from` of "now-6h"', () => {
+              expect(trail.state.history.state.steps[1].trailState.$timeRange?.state.from).toBe('now-6h');
+            });
+
+            it('History step 2 should still have `from` of "now-1h"', () => {
+              expect(trail.state.history.state.steps[2].trailState.$timeRange?.state.from).toBe('now-1h');
+            });
+
+            describe('And then when returning again to step 1', () => {
+              beforeEach(() => {
+                trail.state.history.goBackToStep(1);
+              });
+
+              it('Should set history currentStep to 1', () => {
+                expect(trail.state.history.state.currentStep).toBe(1);
+              });
+
+              it('should sync state with url', () => {
+                expect(locationService.getSearchObject().from).toBe('now-6h');
+              });
+
+              it('History step 1 (parent) should have previous default `from` of "now-6h"', () => {
+                expect(trail.state.history.state.steps[1].trailState.$timeRange?.state.from).toBe('now-6h');
+              });
+
+              it('History step 2 should still have `from` of "now-1h"', () => {
+                expect(trail.state.history.state.steps[2].trailState.$timeRange?.state.from).toBe('now-1h');
+              });
+
+              it('History step 3 should still have `from` of "now-15m"', () => {
+                expect(trail.state.history.state.steps[3].trailState.$timeRange?.state.from).toBe('now-15m');
+              });
+
+              it('Should have time range `from` be set back to "now-6h"', () => {
+                expect(trail.state.$timeRange?.state.from).toBe('now-6h');
+              });
+            });
+          });
+        });
+      });
+
+      function getFilterVar() {
+        const variable = sceneGraph.lookupVariable(VAR_FILTERS, trail);
+        if (variable instanceof AdHocFiltersVariable) {
+          return variable;
+        }
+        throw new Error('getFilterVar failed');
+      }
+
+      function getStepFilterVar(step: number) {
+        const variable = trail.state.history.state.steps[step].trailState.$variables?.getByName(VAR_FILTERS);
+        if (variable instanceof AdHocFiltersVariable) {
+          return variable;
+        }
+        throw new Error(`getStepFilterVar failed for step ${step}`);
+      }
+
+      it('Should have default empty filter', () => {
+        expect(getFilterVar().state.filters.length).toBe(0);
+      });
+
+      describe('And when changing the filter to zone=a', () => {
+        beforeEach(() => {
+          getFilterVar().setState({ filters: [{ key: 'zone', operator: '=', value: 'a' }] });
+        });
+
+        it('should sync state with url', () => {
+          expect(decodeURIComponent(locationService.getSearchObject()['var-filters']?.toString()!)).toBe('zone|=|a');
+        });
+
+        it('should add history step', () => {
+          expect(trail.state.history.state.steps[2].type).toBe('filters');
+        });
+
+        it('Should set history currentStep to 2', () => {
+          expect(trail.state.history.state.currentStep).toBe(2);
+        });
+
+        it('Should set history step 2 parentIndex to 1', () => {
+          expect(trail.state.history.state.steps[2].parentIndex).toBe(1);
+        });
+
+        it('Should have filter be updated to "zone=a"', () => {
+          expect(getFilterVar().state.filters[0].key).toBe('zone');
+          expect(getFilterVar().state.filters[0].value).toBe('a');
+        });
+
+        it('Previous history step should have empty filter', () => {
+          expect(getStepFilterVar(1).state.filters.length).toBe(0);
+        });
+
+        it('Current history step should have new filter zone=a', () => {
+          expect(getStepFilterVar(2).state.filters[0].key).toBe('zone');
+          expect(getStepFilterVar(2).state.filters[0].value).toBe('a');
+        });
+
+        describe('And when traversing back to step 1', () => {
+          beforeEach(() => {
+            trail.state.history.goBackToStep(1);
+          });
+
+          it('Should set history currentStep to 1', () => {
+            expect(trail.state.history.state.currentStep).toBe(1);
+          });
+
+          it('should sync state with url', () => {
+            expect(locationService.getSearchObject()['var-filters']).toBe('');
+          });
+
+          it('Should have filters set back to empty', () => {
+            expect(getFilterVar().state.filters.length).toBe(0);
+          });
+
+          describe('And when changing the filter to zone=b', () => {
+            beforeEach(() => {
+              getFilterVar().setState({ filters: [{ key: 'zone', operator: '=', value: 'b' }] });
+            });
+
+            it('should sync state with url', () => {
+              expect(decodeURIComponent(locationService.getSearchObject()['var-filters']?.toString()!)).toBe(
+                'zone|=|b'
+              );
+            });
+
+            it('should add history step', () => {
+              expect(trail.state.history.state.steps[3].type).toBe('filters');
+            });
+
+            it('Should set history currentStep to 3', () => {
+              expect(trail.state.history.state.currentStep).toBe(3);
+            });
+
+            it('Should set history step 3 parentIndex to 1', () => {
+              expect(trail.state.history.state.steps[3].parentIndex).toBe(1);
+            });
+
+            it('Should have filter be updated to "zone=b"', () => {
+              expect(getFilterVar().state.filters[0].key).toBe('zone');
+              expect(getFilterVar().state.filters[0].value).toBe('b');
+            });
+
+            it('Parent history step 1 should still have empty filter', () => {
+              expect(getStepFilterVar(1).state.filters.length).toBe(0);
+            });
+
+            it('History step 2 should still have old filter zone=a', () => {
+              expect(getStepFilterVar(2).state.filters[0].key).toBe('zone');
+              expect(getStepFilterVar(2).state.filters[0].value).toBe('a');
+            });
+
+            it('Current history step 3 should have new filter zone=b', () => {
+              expect(getStepFilterVar(3).state.filters[0].key).toBe('zone');
+              expect(getStepFilterVar(3).state.filters[0].value).toBe('b');
+            });
+
+            describe('And then when returning again to step 1', () => {
+              beforeEach(() => {
+                trail.state.history.goBackToStep(1);
+              });
+
+              it('Should set history currentStep to 1', () => {
+                expect(trail.state.history.state.currentStep).toBe(1);
+              });
+
+              it('should sync state with url', () => {
+                expect(locationService.getSearchObject()['var-filters']).toBe('');
+              });
+
+              it('Should have filters set back to empty', () => {
+                expect(getFilterVar().state.filters.length).toBe(0);
+              });
+
+              it('History step 1 should still have empty filter', () => {
+                expect(getStepFilterVar(1).state.filters.length).toBe(0);
+              });
+
+              it('History step 2 should still have old filter zone=a', () => {
+                expect(getStepFilterVar(2).state.filters[0].key).toBe('zone');
+                expect(getStepFilterVar(2).state.filters[0].value).toBe('a');
+              });
+
+              it('History step 3 should have new filter zone=b', () => {
+                expect(getStepFilterVar(3).state.filters[0].key).toBe('zone');
+                expect(getStepFilterVar(3).state.filters[0].value).toBe('b');
+              });
+            });
+          });
+        });
+      });
     });
 
     describe('When going back to history step 1', () => {

public/app/features/trails/DataTrail.tsx

@@ -18,6 +18,7 @@ import {
   SceneRefreshPicker,
   SceneTimePicker,
   SceneTimeRange,
+  sceneUtils,
   SceneVariable,
   SceneVariableSet,
   VariableDependencyConfig,
@@ -26,7 +27,7 @@ import {
 import { useStyles2 } from '@grafana/ui';
 
 import { DataTrailSettings } from './DataTrailSettings';
-import { DataTrailHistory, DataTrailHistoryStep } from './DataTrailsHistory';
+import { DataTrailHistory } from './DataTrailsHistory';
 import { MetricScene } from './MetricScene';
 import { MetricSelectScene } from './MetricSelectScene';
 import { MetricsHeader } from './MetricsHeader';
@@ -81,59 +82,43 @@ export class DataTrail extends SceneObjectBase<DataTrailState> {
     // Some scene elements publish this
     this.subscribeToEvent(MetricSelectedEvent, this._handleMetricSelectedEvent.bind(this));
 
-    // Pay attention to changes in history (i.e., changing the step)
-    this.state.history.subscribeToState((newState, oldState) => {
-      const oldNumberOfSteps = oldState.steps.length;
-      const newNumberOfSteps = newState.steps.length;
-
-      const newStepWasAppended = newNumberOfSteps > oldNumberOfSteps;
-
-      if (newStepWasAppended) {
-        // A new step is a significant change. Update the URL to match the new state.
-        this.syncTrailToUrl();
-        // In order for the `useBookmarkState` to re-evaluate after a new step was made:
-        this.forceRender();
-        // Do nothing else because the step state is already up to date -- it created a new step!
-        return;
-      }
-
-      if (oldState.currentStep === newState.currentStep) {
-        // The same step was clicked on -- no need to change anything.
-        return;
-      }
-
-      // History changed because a different node was selected
-      const step = newState.steps[newState.currentStep];
-
-      if (!step) {
-        return;
-      }
-
-      this.goBackToStep(step);
-    });
-
     const filtersVariable = sceneGraph.lookupVariable(VAR_FILTERS, this);
-    const stateSubscription =
-      filtersVariable instanceof AdHocFiltersVariable &&
-      filtersVariable?.subscribeToState((newState, prevState) => {
-        if (!this._addingFilterWithoutReportingInteraction) {
-          reportChangeInLabelFilters(newState.filters, prevState.filters);
-        }
-      });
+    if (filtersVariable instanceof AdHocFiltersVariable) {
+      this._subs.add(
+        filtersVariable?.subscribeToState((newState, prevState) => {
+          if (!this._addingFilterWithoutReportingInteraction) {
+            reportChangeInLabelFilters(newState.filters, prevState.filters);
+          }
+        })
+      );
+    }
+
+    this.enableUrlSync();
 
     return () => {
+      this.disableUrlSync();
+
       if (!this.state.embedded) {
         getTrailStore().setRecentTrail(this);
       }
-      if (stateSubscription) {
-        stateSubscription?.unsubscribe();
-      }
     };
   }
 
+  private enableUrlSync() {
+    if (!this.state.embedded) {
+      getUrlSyncManager().initSync(this);
+    }
+  }
+
+  private disableUrlSync() {
+    if (!this.state.embedded) {
+      getUrlSyncManager().cleanUp(this);
+    }
+  }
+
   protected _variableDependency = new VariableDependencyConfig(this, {
     variableNames: [VAR_DATASOURCE],
-    onReferencedVariableValueChanged: async (variable: SceneVariable) => {
+    onReferencedVariableValueChanged: (variable: SceneVariable) => {
       const { name } = variable.state;
       if (name === VAR_DATASOURCE) {
         this.datasourceHelper.reset();
@@ -153,13 +138,13 @@ export class DataTrail extends SceneObjectBase<DataTrailState> {
     }
 
     this._addingFilterWithoutReportingInteraction = true;
-    variable.setState({
-      filters: [...variable.state.filters, filter],
-    });
+
+    variable.setState({ filters: [...variable.state.filters, filter] });
+
     this._addingFilterWithoutReportingInteraction = false;
   }
-  private _addingFilterWithoutReportingInteraction = false;
 
+  private _addingFilterWithoutReportingInteraction = false;
   private datasourceHelper = new MetricDatasourceHelper(this);
 
   public getMetricMetadata(metric?: string) {
@@ -170,25 +155,21 @@ export class DataTrail extends SceneObjectBase<DataTrailState> {
     return this.getMetricMetadata(this.state.metric);
   }
 
-  private goBackToStep(step: DataTrailHistoryStep) {
-    if (!step.trailState.metric) {
-      step.trailState.metric = undefined;
-    }
+  public restoreFromHistoryStep(state: DataTrailState) {
+    this.disableUrlSync();
 
-    this.setState(step.trailState);
-    this.syncTrailToUrl();
-  }
-
-  private syncTrailToUrl() {
-    if (this.state.embedded) {
-      // Embedded trails should not be altering the URL
-      return;
-    }
+    this.setState(
+      sceneUtils.cloneSceneObjectState(state, {
+        history: this.state.history,
+        metric: !state.metric ? undefined : state.metric,
+      })
+    );
 
     const urlState = getUrlSyncManager().getUrlState(this);
     const fullUrl = urlUtil.renderUrl(locationService.getLocation().pathname, urlState);
+    locationService.replace(fullUrl);
 
-    locationService.replace(encodeURI(fullUrl));
+    this.enableUrlSync();
   }
 
   private _handleMetricSelectedEvent(evt: MetricSelectedEvent) {

public/app/features/trails/DataTrailsHistory.tsx

@@ -124,14 +124,17 @@ export class DataTrailHistory extends SceneObjectBase<DataTrailsHistoryState> {
       return;
     }
 
-    this.stepTransitionInProgress = true;
     const step = this.state.steps[stepIndex];
     const type = step.type === 'metric' && step.trailState.metric === undefined ? 'metric-clear' : step.type;
+
     reportExploreMetrics('history_step_clicked', { type });
 
+    this.stepTransitionInProgress = true;
     this.setState({ currentStep: stepIndex });
-    // The URL will update
 
+    getTrailFor(this).restoreFromHistoryStep(step.trailState);
+
+    // The URL will update
     this.stepTransitionInProgress = false;
   }