diff --git a/public/app/features/trails/DataTrail.tsx b/public/app/features/trails/DataTrail.tsx index e2ac91a180e..4b3820f5f96 100644 --- a/public/app/features/trails/DataTrail.tsx +++ b/public/app/features/trails/DataTrail.tsx @@ -95,12 +95,17 @@ export class DataTrail extends SceneObjectBase { 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); }; } diff --git a/public/app/features/trails/DataTrailsApp.tsx b/public/app/features/trails/DataTrailsApp.tsx index 9622851ca5a..9f2511306be 100644 --- a/public/app/features/trails/DataTrailsApp.tsx +++ b/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({ diff --git a/public/app/features/trails/DataTrailsHistory.tsx b/public/app/features/trails/DataTrailsHistory.tsx index 5c08c590e4e..539174d277f 100644 --- a/public/app/features/trails/DataTrailsHistory.tsx +++ b/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; diff --git a/public/app/features/trails/TrailStore/TrailStore.test.ts b/public/app/features/trails/TrailStore/TrailStore.test.ts index 1b6c1447fa6..8fe31ff9181 100644 --- a/public/app/features/trails/TrailStore/TrailStore.test.ts +++ b/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('[]'); + }); + }); }); diff --git a/public/app/features/trails/TrailStore/TrailStore.ts b/public/app/features/trails/TrailStore/TrailStore.ts index 082a2a80099..02c7cd92446 100644 --- a/public/app/features/trails/TrailStore/TrailStore.ts +++ b/public/app/features/trails/TrailStore/TrailStore.ts @@ -26,12 +26,12 @@ export interface SerializedTrail { export class TrailStore { private _recent: Array> = []; private _bookmarks: Array> = []; - 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; -} diff --git a/public/app/features/trails/TrailStore/useBookmarkState.ts b/public/app/features/trails/TrailStore/useBookmarkState.ts index 632d26e0695..516c73018e3 100644 --- a/public/app/features/trails/TrailStore/useBookmarkState.ts +++ b/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);