datatrails: fix bookmark/recent trail detection, prevent duplications, save trail on browser close/reload (#85677)

* fix: persistence trail detection, save on unload

- fixes detection on bookmarks and recents when current step isn't final
- now save current trail on browser close or reload (unload)
- refresh page or return to URL that corresponds to a recent trail will
   resume that trail instead of creating a duplicate recent trail
- do not create a recent trail out of an empty starting trail

* fix: bookmarks status after making new step

- clone bookmark trail state to prevent it from being changed by user
- re-evaluate bookmark status after creating new step

public/app/features/trails/DataTrail.tsx

@@ -95,12 +95,17 @@ export class DataTrail extends SceneObjectBase<DataTrailState> {
 
     this.enableUrlSync();
 
+    // Save the current trail as a recent if the browser closes or reloads
+    const saveRecentTrail = () => getTrailStore().setRecentTrail(this);
+    window.addEventListener('unload', saveRecentTrail);
+
     return () => {
       this.disableUrlSync();
 
       if (!this.state.embedded) {
-        getTrailStore().setRecentTrail(this);
+        saveRecentTrail();
       }
+      window.removeEventListener('unload', saveRecentTrail);
     };
   }
 

public/app/features/trails/DataTrailsApp.tsx

@@ -75,14 +75,6 @@ function DataTrailView({ trail }: { trail: DataTrail }) {
 
   useEffect(() => {
     if (!isInitialized) {
-      // Set the initial state based on the URL.
-      getUrlSyncManager().initSync(trail);
-      // Any further changes to the state should occur directly to the state, not through the URL.
-      // We want to stop automatically syncing the URL state (and vice versa) to the trail after this point.
-      // Moving forward in the lifecycle of the trail, we will make explicit calls to trail.syncTrailToUrl()
-      // so we can ensure the URL is kept up to date at key points.
-      getUrlSyncManager().cleanUp(trail);
-
       getTrailStore().setRecentTrail(trail);
       setIsInitialized(true);
     }
@@ -100,7 +92,7 @@ let dataTrailsApp: DataTrailsApp;
 export function getDataTrailsApp() {
   if (!dataTrailsApp) {
     dataTrailsApp = new DataTrailsApp({
-      trail: newMetricsTrail(),
+      trail: getInitialTrail(),
       home: new DataTrailsHome({}),
     });
   }
@@ -108,6 +100,34 @@ export function getDataTrailsApp() {
   return dataTrailsApp;
 }
 
+/**
+ * Get the initial trail for the app to work with based on the current URL
+ *
+ * It will either be a new trail that will be started based on the state represented
+ * in the URL parameters, or it will be the most recently used trail (according to the trail store)
+ * which has its current history step matching the URL parameters.
+ *
+ * The reason for trying to reinitialize from the recent trail is to resolve an issue
+ * where refreshing the browser would wipe the step history. This allows you to preserve
+ * it between browser refreshes, or when reaccessing the same URL.
+ */
+function getInitialTrail() {
+  const newTrail = newMetricsTrail();
+
+  // Set the initial state of the newTrail based on the URL,
+  // In case we are initializing from an externally created URL or a page reload
+  getUrlSyncManager().initSync(newTrail);
+  // Remove the URL sync for now. It will be restored on the trail if it is activated.
+  getUrlSyncManager().cleanUp(newTrail);
+
+  // If one of the recent trails is a match to the newTrail derived from the current URL,
+  // let's restore that trail so that a page refresh doesn't create a new trail.
+  const recentMatchingTrail = getTrailStore().findMatchingRecentTrail(newTrail)?.resolve();
+
+  // If there is a matching trail, initialize with that. Otherwise, use the new trail.
+  return recentMatchingTrail || newTrail;
+}
+
 function getStyles(theme: GrafanaTheme2) {
   return {
     customPage: css({

public/app/features/trails/DataTrailsHistory.tsx

@@ -23,6 +23,10 @@ export interface DataTrailsHistoryState extends SceneObjectState {
   steps: DataTrailHistoryStep[];
 }
 
+export function isDataTrailsHistoryState(state: SceneObjectState): state is DataTrailsHistoryState {
+  return 'currentStep' in state && 'steps' in state;
+}
+
 export interface DataTrailHistoryStep {
   description: string;
   type: TrailStepType;

public/app/features/trails/TrailStore/TrailStore.test.ts

@@ -165,7 +165,184 @@ describe('TrailStore', () => {
       // There should now be two trails
       expect(store.recent.length).toBe(2);
     });
+
+    test('deserializeTrail must show state of current step when not last step', () => {
+      const trailSerialized: SerializedTrail = {
+        history: [
+          history[0],
+          history[1],
+          {
+            ...history[1],
+            urlValues: {
+              ...history[1].urlValues,
+              metric: 'something_else',
+            },
+            parentIndex: 1,
+          },
+        ],
+        currentStep: 1,
+      };
+
+      // @ts-ignore #2341 -- deliberately access private method to construct trail object for testing purposes
+      const trail = getTrailStore()._deserializeTrail(trailSerialized);
+
+      //
+      expect(trail.state.metric).not.toEqual('something_else');
+      expect(trail.state.metric).toEqual(history[1].urlValues.metric);
+    });
   });
+
+  describe('Initialize store with one recent trail with non final current step', () => {
+    const history: SerializedTrail['history'] = [
+      {
+        urlValues: {
+          from: 'now-1h',
+          to: 'now',
+          'var-ds': 'ds',
+          'var-filters': [],
+          refresh: '',
+        },
+        type: 'start',
+        description: 'Test',
+        parentIndex: -1,
+      },
+      {
+        urlValues: {
+          metric: 'current_metric',
+          from: 'now-1h',
+          to: 'now',
+          'var-ds': 'ds',
+          'var-filters': [],
+          refresh: '',
+        },
+        type: 'metric',
+        description: 'Test',
+        parentIndex: 0,
+      },
+      {
+        urlValues: {
+          metric: 'final_metric',
+          from: 'now-1h',
+          to: 'now',
+          'var-ds': 'ds',
+          'var-filters': [],
+          refresh: '',
+        },
+        type: 'metric',
+        description: 'Test',
+        parentIndex: 1,
+      },
+    ];
+
+    beforeEach(() => {
+      localStorage.clear();
+      localStorage.setItem(RECENT_TRAILS_KEY, JSON.stringify([{ history, currentStep: 1 }]));
+      getTrailStore().load();
+    });
+
+    it('should accurately load recent trails', () => {
+      const store = getTrailStore();
+      expect(store.recent.length).toBe(1);
+      const trail = store.recent[0].resolve();
+      expect(trail.state.history.state.steps.length).toBe(3);
+      expect(trail.state.history.state.steps[0].type).toBe('start');
+      expect(trail.state.history.state.steps[1].type).toBe('metric');
+      expect(trail.state.history.state.steps[1].trailState.metric).toBe('current_metric');
+      expect(trail.state.history.state.steps[2].type).toBe('metric');
+      expect(trail.state.history.state.steps[2].trailState.metric).toBe('final_metric');
+      expect(trail.state.history.state.currentStep).toBe(1);
+    });
+
+    it('should have no bookmarked trails', () => {
+      const store = getTrailStore();
+      expect(store.bookmarks.length).toBe(0);
+    });
+
+    describe('Add a new recent trail with equivalent current step state', () => {
+      const store = getTrailStore();
+
+      const duplicateTrailSerialized: SerializedTrail = {
+        history: [
+          history[0],
+          history[1],
+          history[2],
+          {
+            ...history[2],
+            urlValues: {
+              ...history[1].urlValues,
+              metric: 'different_metric_in_the_middle',
+            },
+          },
+          {
+            ...history[1],
+          },
+        ],
+        currentStep: 4,
+      };
+
+      beforeEach(() => {
+        // We expect the initialized trail to be there
+        expect(store.recent.length).toBe(1);
+        expect(store.recent[0].resolve().state.history.state.steps.length).toBe(3);
+
+        // @ts-ignore #2341 -- deliberately access private method to construct trail object for testing purposes
+        const duplicateTrail = store._deserializeTrail(duplicateTrailSerialized);
+        store.setRecentTrail(duplicateTrail);
+      });
+
+      it('should still be only one recent trail', () => {
+        expect(store.recent.length).toBe(1);
+      });
+
+      it('it should only contain the new trail', () => {
+        const newRecentTrail = store.recent[0].resolve();
+        expect(newRecentTrail.state.history.state.steps.length).toBe(duplicateTrailSerialized.history.length);
+
+        // @ts-ignore #2341 -- deliberately access private method to construct trail object for testing purposes
+        const newRecent = store._serializeTrail(newRecentTrail);
+        expect(newRecent.currentStep).toBe(duplicateTrailSerialized.currentStep);
+        expect(newRecent.history.length).toBe(duplicateTrailSerialized.history.length);
+      });
+    });
+
+    it.each([
+      ['metric', 'different_metric'],
+      ['from', 'now-1y'],
+      ['to', 'now-30m'],
+      ['var-ds', '1234'],
+      ['var-groupby', 'job'],
+      ['var-filters', 'cluster|=|dev-eu-west-2'],
+    ])(`new recent trails with a different '%p' value should insert new entry`, (key, differentValue) => {
+      const store = getTrailStore();
+      // We expect the initialized trail to be there
+      expect(store.recent.length).toBe(1);
+
+      const differentTrailSerialized: SerializedTrail = {
+        history: [
+          history[0],
+          history[1],
+          history[2],
+          {
+            ...history[2],
+            urlValues: {
+              ...history[1].urlValues,
+              [key]: differentValue,
+            },
+            parentIndex: 1,
+          },
+        ],
+        currentStep: 3,
+      };
+
+      // @ts-ignore #2341 -- deliberately access private method to construct trail object for testing purposes
+      const differentTrail = store._deserializeTrail(differentTrailSerialized);
+      store.setRecentTrail(differentTrail);
+
+      // There should now be two trails
+      expect(store.recent.length).toBe(2);
+    });
+  });
+
   describe('Initialize store with one bookmark trail', () => {
     beforeEach(() => {
       localStorage.clear();
@@ -264,4 +441,111 @@ describe('TrailStore', () => {
       expect(localStorage.getItem(BOOKMARKED_TRAILS_KEY)).toBe('[]');
     });
   });
+
+  describe('Initialize store with one bookmark trail not on final step', () => {
+    beforeEach(() => {
+      localStorage.clear();
+      localStorage.setItem(
+        BOOKMARKED_TRAILS_KEY,
+        JSON.stringify([
+          {
+            history: [
+              {
+                urlValues: {
+                  from: 'now-1h',
+                  to: 'now',
+                  'var-ds': 'prom-mock',
+                  'var-filters': [],
+                  refresh: '',
+                },
+                type: 'start',
+              },
+              {
+                urlValues: {
+                  metric: 'bookmarked_metric',
+                  from: 'now-1h',
+                  to: 'now',
+                  'var-ds': 'prom-mock',
+                  'var-filters': [],
+                  refresh: '',
+                },
+                type: 'time',
+              },
+              {
+                urlValues: {
+                  metric: 'some_other_metric',
+                  from: 'now-1h',
+                  to: 'now',
+                  'var-ds': 'prom-mock',
+                  'var-filters': [],
+                  refresh: '',
+                },
+                type: 'metric',
+              },
+            ],
+            currentStep: 1,
+          },
+        ])
+      );
+      getTrailStore().load();
+    });
+
+    const store = getTrailStore();
+
+    it('should have no recent trails', () => {
+      expect(store.recent.length).toBe(0);
+    });
+
+    it('should accurately load bookmarked trails', () => {
+      expect(store.bookmarks.length).toBe(1);
+      const trail = store.bookmarks[0].resolve();
+      expect(trail.state.history.state.steps.length).toBe(3);
+      expect(trail.state.history.state.steps[0].type).toBe('start');
+      expect(trail.state.history.state.steps[1].type).toBe('time');
+      expect(trail.state.history.state.steps[2].type).toBe('metric');
+    });
+
+    it('should save a new recent trail based on the bookmark', () => {
+      expect(store.recent.length).toBe(0);
+      const trail = store.bookmarks[0].resolve();
+      store.setRecentTrail(trail);
+      expect(store.recent.length).toBe(1);
+    });
+
+    it('should be able to obtain index of bookmark', () => {
+      const trail = store.bookmarks[0].resolve();
+      const index = store.getBookmarkIndex(trail);
+      expect(index).toBe(0);
+    });
+
+    it('index should be undefined for removed bookmarks', () => {
+      const trail = store.bookmarks[0].resolve();
+      store.removeBookmark(0);
+      const index = store.getBookmarkIndex(trail);
+      expect(index).toBe(undefined);
+    });
+
+    it('index should be undefined for a trail that has changed since it was bookmarked', () => {
+      const trail = store.bookmarks[0].resolve();
+      trail.setState({ metric: 'something_completely_different' });
+      const index = store.getBookmarkIndex(trail);
+      expect(index).toBe(undefined);
+    });
+
+    it('should be able to obtain index of a bookmark for a trail that changed back to bookmarked state', () => {
+      const trail = store.bookmarks[0].resolve();
+      trail.setState({ metric: 'something_completely_different' });
+      expect(store.getBookmarkIndex(trail)).toBe(undefined);
+      trail.setState({ metric: 'bookmarked_metric' });
+      expect(store.getBookmarkIndex(trail)).toBe(0);
+    });
+
+    it('should remove a bookmark', () => {
+      expect(store.bookmarks.length).toBe(1);
+      store.removeBookmark(0);
+      expect(store.bookmarks.length).toBe(0);
+      jest.advanceTimersByTime(2000);
+      expect(localStorage.getItem(BOOKMARKED_TRAILS_KEY)).toBe('[]');
+    });
+  });
 });

public/app/features/trails/TrailStore/TrailStore.ts

@@ -26,12 +26,12 @@ export interface SerializedTrail {
 export class TrailStore {
   private _recent: Array<SceneObjectRef<DataTrail>> = [];
   private _bookmarks: Array<SceneObjectRef<DataTrail>> = [];
-  private _save;
+  private _save: () => void;
 
   constructor() {
     this.load();
 
-    this._save = debounce(() => {
+    const doSave = () => {
       const serializedRecent = this._recent
         .slice(0, MAX_RECENT_TRAILS)
         .map((trail) => this._serializeTrail(trail.resolve()));
@@ -39,7 +39,15 @@ export class TrailStore {
 
       const serializedBookmarks = this._bookmarks.map((trail) => this._serializeTrail(trail.resolve()));
       localStorage.setItem(BOOKMARKED_TRAILS_KEY, JSON.stringify(serializedBookmarks));
-    }, 1000);
+    };
+
+    this._save = debounce(doSave, 1000);
+
+    window.addEventListener('beforeunload', (ev) => {
+      // Before closing or reloading the page, we want to remove the debounce from `_save` so that
+      // any calls to is on event `unload` are actualized. Debouncing would cause a delay until after the page has been unloaded.
+      this._save = doSave;
+    });
   }
 
   private _loadFromStorage(key: string) {
@@ -70,6 +78,8 @@ export class TrailStore {
 
     const currentStep = t.currentStep ?? trail.state.history.state.steps.length - 1;
     trail.state.history.setState({ currentStep });
+    // The state change listeners aren't activated yet, so maually change to the current step state
+    trail.setState(trail.state.history.state.steps[currentStep].trailState);
 
     return trail;
   }
@@ -107,29 +117,45 @@ export class TrailStore {
     this._refreshBookmarkIndexMap();
   }
 
-  setRecentTrail(trail: DataTrail) {
-    this._recent = this._recent.filter((t) => t !== trail.getRef());
+  setRecentTrail(recentTrail: DataTrail) {
+    const { steps } = recentTrail.state.history.state;
+    if (steps.length === 0 || (steps.length === 1 && steps[0].type === 'start')) {
+      // We do not set an uninitialized trail, or a single node "start" trail as recent
+      return;
+    }
 
-    // Check if any existing "recent" entries have equivalent 'current' urlValue to the new trail
-    const newTrailUrlValues = getCurrentUrlValues(this._serializeTrail(trail)) || {};
+    // Remove the `recentTrail` from the list if it already exists there
+    this._recent = this._recent.filter((t) => t !== recentTrail.getRef());
+
+    // Check if any existing "recent" entries have equivalent urlState to the new recentTrail
+    const recentUrlState = getUrlStateForComparison(recentTrail); //
     this._recent = this._recent.filter((t) => {
       // Use the current step urlValues to filter out equivalent states
-      const urlValues = getCurrentUrlValues(this._serializeTrail(t.resolve()));
+      const urlState = getUrlStateForComparison(t.resolve());
       // Only keep trails with sufficiently unique urlValues on their current step
-      return !isEqual(newTrailUrlValues, urlValues);
+      return !isEqual(recentUrlState, urlState);
     });
 
-    this._recent.unshift(trail.getRef());
+    this._recent.unshift(recentTrail.getRef());
     this._save();
   }
 
+  findMatchingRecentTrail(trail: DataTrail) {
+    const matchUrlState = getUrlStateForComparison(trail);
+    return this._recent.find((t) => {
+      const urlState = getUrlStateForComparison(t.resolve());
+      return isEqual(matchUrlState, urlState);
+    });
+  }
+
   // Bookmarked Trails
   get bookmarks() {
     return this._bookmarks;
   }
 
   addBookmark(trail: DataTrail) {
-    this._bookmarks.unshift(trail.getRef());
+    const bookmark = new DataTrail(sceneUtils.cloneSceneObjectState(trail.state));
+    this._bookmarks.unshift(bookmark.getRef());
     this._refreshBookmarkIndexMap();
     this._save();
     dispatch(notifyApp(createBookmarkSavedNotification()));
@@ -155,6 +181,7 @@ export class TrailStore {
     this._bookmarkIndexMap.clear();
     this._bookmarks.forEach((bookmarked, index) => {
       const trail = bookmarked.resolve();
+
       const key = getBookmarkKey(trail);
       // If there are duplicate bookmarks, the latest index will be kept
       this._bookmarkIndexMap.set(key, index);
@@ -162,15 +189,24 @@ export class TrailStore {
   }
 }
 
-function getBookmarkKey(trail: DataTrail) {
+function getUrlStateForComparison(trail: DataTrail) {
   const urlState = getUrlSyncManager().getUrlState(trail);
-  // Not part of state
+  // Make a few corrections
+
+  // Omit some URL parameters that are not useful for state comparison
   delete urlState.actionView;
+  delete urlState.layout;
+
   // Populate defaults
   if (urlState['var-groupby'] === '') {
     urlState['var-groupby'] = '$__all';
   }
-  const key = JSON.stringify(urlState);
+
+  return urlState;
+}
+
+function getBookmarkKey(trail: DataTrail) {
+  const key = JSON.stringify(getUrlStateForComparison(trail));
   return key;
 }
 
@@ -182,7 +218,3 @@ export function getTrailStore(): TrailStore {
 
   return store;
 }
-
-function getCurrentUrlValues({ history, currentStep }: SerializedTrail) {
-  return history[currentStep]?.urlValues || history.at(-1)?.urlValues;
-}

public/app/features/trails/TrailStore/useBookmarkState.ts

@@ -1,6 +1,9 @@
-import { useState } from 'react';
+import { useEffect, useState } from 'react';
+
+import { SceneObjectStateChangedEvent } from '@grafana/scenes';
 
 import { DataTrail } from '../DataTrail';
+import { isDataTrailsHistoryState } from '../DataTrailsHistory';
 import { reportExploreMetrics } from '../interactions';
 
 import { getTrailStore } from './TrailStore';
@@ -14,6 +17,18 @@ export function useBookmarkState(trail: DataTrail) {
 
   const [bookmarkIndex, setBookmarkIndex] = useState(indexOnRender);
 
+  useEffect(() => {
+    const sub = trail.subscribeToEvent(SceneObjectStateChangedEvent, ({ payload: { prevState, newState } }) => {
+      if (isDataTrailsHistoryState(prevState) && isDataTrailsHistoryState(newState)) {
+        if (newState.steps.length > prevState.steps.length) {
+          // When we add new steps, we need to re-evaluate whether or not it is still a bookmark
+          setBookmarkIndex(getTrailStore().getBookmarkIndex(trail));
+        }
+      }
+    });
+    return () => sub.unsubscribe();
+  }, [trail]);
+
   // Check if index changed and force a re-render
   if (indexOnRender !== bookmarkIndex) {
     setBookmarkIndex(indexOnRender);