From 137061d1d5e3208d09dcf326149e55562577c4a0 Mon Sep 17 00:00:00 2001 From: Darren Janeczek <38694490+darrenjaneczek@users.noreply.github.com> Date: Thu, 28 Mar 2024 09:59:16 -0400 Subject: [PATCH] datatrails: fix stability issues between conflict between browser history, URL sync, and trail history (#85134) * fix: conflict between browser history and trail history - ensure the back button or url changes don't generate trail steps - ensure label breakdown TextLinks which appear on the summary tab work in embedded mode --- .../trails/ActionTabs/MetricOverviewScene.tsx | 42 +++++++++---------- public/app/features/trails/DataTrail.test.tsx | 25 +++++++++-- public/app/features/trails/DataTrail.tsx | 32 +++++++------- public/app/features/trails/DataTrailsApp.tsx | 9 +++- .../app/features/trails/DataTrailsHistory.tsx | 14 +++++-- 5 files changed, 77 insertions(+), 45 deletions(-) diff --git a/public/app/features/trails/ActionTabs/MetricOverviewScene.tsx b/public/app/features/trails/ActionTabs/MetricOverviewScene.tsx index 7dfd896d2a7..799ee2c2b13 100644 --- a/public/app/features/trails/ActionTabs/MetricOverviewScene.tsx +++ b/public/app/features/trails/ActionTabs/MetricOverviewScene.tsx @@ -12,8 +12,9 @@ import { Stack, Text, TextLink } from '@grafana/ui'; import { PromMetricsMetadataItem } from '../../../plugins/datasource/prometheus/types'; import { ALL_VARIABLE_VALUE } from '../../variables/constants'; +import { MetricScene } from '../MetricScene'; import { StatusWrapper } from '../StatusWrapper'; -import { TRAILS_ROUTE, VAR_DATASOURCE_EXPR, VAR_GROUP_BY } from '../shared'; +import { VAR_DATASOURCE_EXPR, VAR_GROUP_BY } from '../shared'; import { getMetricSceneFor, getTrailFor } from '../utils'; import { getLabelOptions } from './utils'; @@ -91,26 +92,25 @@ export class MetricOverviewScene extends SceneObjectBase Labels {labelOptions.length === 0 && 'Unable to fetch labels.'} - {labelOptions.map((l) => - getTrailFor(model).state.embedded ? ( - // Do not render as TextLink when in embedded mode, as any direct URL - // manipulation will take the browser out out of the current page. -
{l.label}
- ) : ( - - {l.label!} - - ) - )} + {labelOptions.map((l) => ( + { + event.preventDefault(); + event.stopPropagation(); + sceneGraph.getAncestor(model, MetricScene).setActionView('breakdown'); + const groupByVar = sceneGraph.lookupVariable(VAR_GROUP_BY, model); + if (groupByVar instanceof QueryVariable) { + groupByVar.setState({ value: l.value }); + } + return false; + }} + > + {l.label!} + + ))} diff --git a/public/app/features/trails/DataTrail.test.tsx b/public/app/features/trails/DataTrail.test.tsx index 31c6869ca63..21b24017da6 100644 --- a/public/app/features/trails/DataTrail.test.tsx +++ b/public/app/features/trails/DataTrail.test.tsx @@ -1,5 +1,4 @@ import { locationService, setDataSourceSrv } from '@grafana/runtime'; -import { getUrlSyncManager } from '@grafana/scenes'; import { MockDataSourceSrv, mockDataSource } from '../alerting/unified/mocks'; import { DataSourceType } from '../alerting/unified/utils/datasource'; @@ -22,13 +21,13 @@ describe('DataTrail', () => { ); }); - describe('Given starting trail with url sync and no url state', () => { + describe('Given starting non-embedded trail with url sync and no url state', () => { let trail: DataTrail; + const preTrailUrl = '/'; beforeEach(() => { trail = new DataTrail({}); - locationService.push('/'); - getUrlSyncManager().initSync(trail); + locationService.push(preTrailUrl); activateFullSceneTree(trail); }); @@ -73,6 +72,15 @@ describe('DataTrail', () => { it('Should set history step 1 parentIndex to 0', () => { expect(trail.state.history.state.steps[1].parentIndex).toBe(0); }); + + describe('And browser back button is pressed', () => { + locationService.getHistory().goBack(); + + it('Should return to original URL', () => { + const { pathname } = locationService.getLocation(); + expect(pathname).toEqual(preTrailUrl); + }); + }); }); describe('When going back to history step 1', () => { @@ -111,6 +119,15 @@ describe('DataTrail', () => { it('Should set history step 3 parent index to 1', () => { expect(trail.state.history.state.steps[3].parentIndex).toBe(1); }); + + describe('And browser back button is pressed', () => { + locationService.getHistory().goBack(); + + it('Should return to original URL', () => { + const { pathname } = locationService.getLocation(); + expect(pathname).toEqual(preTrailUrl); + }); + }); }); }); }); diff --git a/public/app/features/trails/DataTrail.tsx b/public/app/features/trails/DataTrail.tsx index 04b7493dd6a..938d3f179be 100644 --- a/public/app/features/trails/DataTrail.tsx +++ b/public/app/features/trails/DataTrail.tsx @@ -33,7 +33,6 @@ import { MetricsHeader } from './MetricsHeader'; import { getTrailStore } from './TrailStore/TrailStore'; import { MetricDatasourceHelper } from './helpers/MetricDatasourceHelper'; import { MetricSelectedEvent, trailDS, VAR_DATASOURCE, VAR_FILTERS } from './shared'; -import { getUrlForTrail } from './utils'; export interface DataTrailState extends SceneObjectState { topScene?: SceneObject; @@ -89,9 +88,11 @@ export class DataTrail extends SceneObjectBase { 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 because the state is already up to date -- it created a new step! + // Do nothing else because the step state is already up to date -- it created a new step! return; } @@ -103,12 +104,15 @@ export class DataTrail extends SceneObjectBase { // History changed because a different node was selected const step = newState.steps[newState.currentStep]; + if (!step) { + return; + } + this.goBackToStep(step); }); return () => { if (!this.state.embedded) { - getUrlSyncManager().cleanUp(this); getTrailStore().setRecentTrail(this); } }; @@ -135,29 +139,25 @@ export class DataTrail extends SceneObjectBase { } private goBackToStep(step: DataTrailHistoryStep) { - if (!this.state.embedded) { - getUrlSyncManager().cleanUp(this); - } - if (!step.trailState.metric) { step.trailState.metric = undefined; } this.setState(step.trailState); + this.syncTrailToUrl(); + } - if (!this.state.embedded) { - locationService.replace(getUrlForTrail(this)); - - getUrlSyncManager().initSync(this); + private syncTrailToUrl() { + if (this.state.embedded) { + // Embedded trails should not be altering the URL + return; } + const urlState = getUrlSyncManager().getUrlState(this); + locationService.partial(urlState, true); } private _handleMetricSelectedEvent(evt: MetricSelectedEvent) { - if (this.state.embedded) { - this.setState(this.getSceneUpdatesForNewMetricValue(evt.payload)); - } else { - locationService.partial({ metric: evt.payload, actionView: null }); - } + this.setState(this.getSceneUpdatesForNewMetricValue(evt.payload)); // Add metric to adhoc filters baseFilter const filterVar = sceneGraph.lookupVariable(VAR_FILTERS, this); diff --git a/public/app/features/trails/DataTrailsApp.tsx b/public/app/features/trails/DataTrailsApp.tsx index b9e9f48dd08..9622851ca5a 100644 --- a/public/app/features/trails/DataTrailsApp.tsx +++ b/public/app/features/trails/DataTrailsApp.tsx @@ -4,7 +4,7 @@ import { Route, Switch } from 'react-router-dom'; import { GrafanaTheme2, PageLayoutType } from '@grafana/data'; import { locationService } from '@grafana/runtime'; -import { getUrlSyncManager, SceneComponentProps, SceneObjectBase, SceneObjectState } from '@grafana/scenes'; +import { SceneComponentProps, SceneObjectBase, SceneObjectState, getUrlSyncManager } from '@grafana/scenes'; import { useStyles2 } from '@grafana/ui'; import { Page } from 'app/core/components/Page/Page'; @@ -75,7 +75,14 @@ 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); } diff --git a/public/app/features/trails/DataTrailsHistory.tsx b/public/app/features/trails/DataTrailsHistory.tsx index c0c4fd2f6d6..33ba46d9915 100644 --- a/public/app/features/trails/DataTrailsHistory.tsx +++ b/public/app/features/trails/DataTrailsHistory.tsx @@ -30,7 +30,6 @@ export interface DataTrailHistoryStep { } export type TrailStepType = 'filters' | 'time' | 'metric' | 'start'; - export class DataTrailHistory extends SceneObjectBase { public constructor(state: Partial) { super({ steps: state.steps ?? [], currentStep: state.currentStep ?? 0 }); @@ -120,9 +119,13 @@ export class DataTrailHistory extends SceneObjectBase { } public goBackToStep(stepIndex: number) { - this.stepTransitionInProgress = true; + if (stepIndex === this.state.currentStep) { + return; + } + this.stepTransitionInProgress = true; this.setState({ currentStep: stepIndex }); + // The URL will update this.stepTransitionInProgress = false; } @@ -142,10 +145,15 @@ export class DataTrailHistory extends SceneObjectBase { const { ancestry, alternatePredecessorStyle } = useMemo(() => { const ancestry = new Set(); + let cursor = currentStep; while (cursor >= 0) { + const step = steps[cursor]; + if (!step) { + break; + } ancestry.add(cursor); - cursor = steps[cursor].parentIndex; + cursor = step.parentIndex; } const alternatePredecessorStyle = new Map();