From 10628e874164c2a631e5a8d012370db3cb55b2e2 Mon Sep 17 00:00:00 2001 From: Victor Marin <36818606+mdvictor@users.noreply.github.com> Date: Fri, 16 May 2025 16:16:30 +0300 Subject: [PATCH] Dashboards: Refactor URL state preservation functionality when reloading on params change (#104780) * move preserve to scenePage level, fix issues with restoring variables and failing to reload dd * refactor and fix preserve/reload url state * cleanup * lint * lint * reference this PR in comment --- .../pages/DashboardScenePage.tsx | 2 + .../DashboardScenePageStateManager.test.ts | 89 +++++++++++++++++++ .../pages/DashboardScenePageStateManager.ts | 31 ++++++- .../transformSaveModelSchemaV2ToScene.ts | 2 - .../transformSaveModelToScene.ts | 2 - .../utils/dashboardSessionState.test.ts | 87 ++++++++++-------- .../utils/dashboardSessionState.ts | 69 +++++++------- 7 files changed, 211 insertions(+), 71 deletions(-) diff --git a/public/app/features/dashboard-scene/pages/DashboardScenePage.tsx b/public/app/features/dashboard-scene/pages/DashboardScenePage.tsx index 9e883816200..b13ca336072 100644 --- a/public/app/features/dashboard-scene/pages/DashboardScenePage.tsx +++ b/public/app/features/dashboard-scene/pages/DashboardScenePage.tsx @@ -15,6 +15,7 @@ import { DashboardRoutes } from 'app/types'; import { DashboardPrompt } from '../saving/DashboardPrompt'; import { DashboardPreviewBanner } from '../saving/provisioned/DashboardPreviewBanner'; +import { preserveDashboardSceneStateInLocalStorage } from '../utils/dashboardSessionState'; import { getDashboardScenePageStateManager } from './DashboardScenePageStateManager'; @@ -47,6 +48,7 @@ export function DashboardScenePage({ route, queryParams, location }: Props) { } return () => { + preserveDashboardSceneStateInLocalStorage(locationService.getSearch(), uid); stateManager.clearState(); }; diff --git a/public/app/features/dashboard-scene/pages/DashboardScenePageStateManager.test.ts b/public/app/features/dashboard-scene/pages/DashboardScenePageStateManager.test.ts index 0721d2f32cf..c32616a0b9c 100644 --- a/public/app/features/dashboard-scene/pages/DashboardScenePageStateManager.test.ts +++ b/public/app/features/dashboard-scene/pages/DashboardScenePageStateManager.test.ts @@ -719,6 +719,57 @@ describe('DashboardScenePageStateManager v2', () => { fetchDashboardSpy.mockRestore(); }); + it('should not use cache if cache version and current dashboard state version differ', async () => { + const getDashSpy = jest.fn(); + setupDashboardAPI( + { + access: {}, + apiVersion: 'v2alpha1', + kind: 'DashboardWithAccessInfo', + metadata: { + name: 'fake-dash', + creationTimestamp: '', + resourceVersion: '1', + generation: 1, + }, + spec: { ...defaultDashboardV2Spec() }, + }, + getDashSpy + ); + + const loader = new DashboardScenePageStateManagerV2({}); + await loader.loadDashboard({ uid: 'fake-dash', route: DashboardRoutes.Normal }); + + expect(getDashSpy).toHaveBeenCalledTimes(1); + + const mockDashboard: DashboardWithAccessInfo = { + access: {}, + apiVersion: 'v2alpha1', + kind: 'DashboardWithAccessInfo', + metadata: { + name: 'fake-dash', + creationTimestamp: '', + resourceVersion: '1', + generation: 2, + }, + spec: { ...defaultDashboardV2Spec() }, + }; + + const fetchDashboardSpy = jest.spyOn(loader, 'fetchDashboard').mockResolvedValue(mockDashboard); + + // mimic navigating from db1 to db2 and then back to db1, which maintains the cache. but on + // db1 load the initial version will be 1. Since the cache is set we also need to verify against the + // current dashboard state whether we should reload or not + loader.setSceneCache('fake-dash', loader.state.dashboard!.clone({ version: 2 })); + const options = { version: 2, scopes: [], timeRange: { from: 'now-1h', to: 'now' }, variables: {} }; + await loader.reloadDashboard(options); + + expect(fetchDashboardSpy).toHaveBeenCalledTimes(1); + expect(loader.state.dashboard?.state.version).toBe(2); + + fetchDashboardSpy.mockRestore(); + }); + it('should handle errors during reload', async () => { const getDashSpy = jest.fn(); setupDashboardAPI( @@ -943,6 +994,44 @@ describe('UnifiedDashboardScenePageStateManager', () => { expect(manager['activeManager']).toBeInstanceOf(DashboardScenePageStateManagerV2); }); + + it('should not use cache if cache version and current dashboard state version differ in v1', async () => { + const loadDashboardMock = setupLoadDashboardMock({ + dashboard: { uid: 'fake-dash', editable: true, version: 0 }, + meta: {}, + }); + + const manager = new UnifiedDashboardScenePageStateManager({}); + await manager.loadDashboard({ uid: 'fake-dash', route: DashboardRoutes.Normal }); + + expect(loadDashboardMock).toHaveBeenCalledWith('db', '', 'fake-dash', undefined); + expect(manager['activeManager']).toBeInstanceOf(DashboardScenePageStateManager); + + loadDashboardMock.mockClear(); + + const mockDashboard: DashboardDTO = { + dashboard: { + uid: 'fake-dash', + version: 2, + title: 'fake-dash', + } as DashboardDataDTO, + meta: {}, + }; + + const fetchDashboardSpy = jest.spyOn(manager['activeManager'], 'fetchDashboard').mockResolvedValue(mockDashboard); + + // mimic navigating from db1 to db2 and then back to db1, which maintains the cache. but on + // db1 load the initial version will be 1. Since the cache is set we also need to verify against the + // current dashboard state whether we should reload or not + manager.setSceneCache('fake-dash', manager.state.dashboard!.clone({ version: 2 })); + const options = { version: 2, scopes: [], timeRange: { from: 'now-1h', to: 'now' }, variables: {} }; + await manager.reloadDashboard(options); + + expect(fetchDashboardSpy).toHaveBeenCalledTimes(1); + expect(manager.state.dashboard?.state.version).toBe(2); + + fetchDashboardSpy.mockRestore(); + }); }); describe('Home dashboard', () => { diff --git a/public/app/features/dashboard-scene/pages/DashboardScenePageStateManager.ts b/public/app/features/dashboard-scene/pages/DashboardScenePageStateManager.ts index 438c8d5cd73..a7fe4ce73fc 100644 --- a/public/app/features/dashboard-scene/pages/DashboardScenePageStateManager.ts +++ b/public/app/features/dashboard-scene/pages/DashboardScenePageStateManager.ts @@ -522,7 +522,20 @@ export class DashboardScenePageStateManager extends DashboardScenePageStateManag const rsp = await this.fetchDashboard(options); const fromCache = this.getSceneFromCache(options.uid); - if (fromCache && fromCache.state.version === rsp?.dashboard.version) { + // check if cached db version is same as both + // response and current db state. There are scenarios where they can differ + // e.g: when reloadOnParamsChange ff is on the first loaded dashboard could be version 0 + // then on this reload call the rsp increments the version. When the cache is not set + // it creates a new scene based on the new rsp. But if we navigate to another dashboard + // and then back to the initial one, the cache is still set, but the dashboard will be loaded + // again with version 0. Because the cache is set with the incremented version and the rsp on + // reload will match the cached version we return and do nothing, but the set scene is still + // the one for the version 0 dashboard, thus we verify dashboard state version as well + if ( + fromCache && + fromCache.state.version === rsp?.dashboard.version && + fromCache.state.version === this.state.dashboard?.state.version + ) { this.setState({ isLoading: false }); return; } @@ -540,6 +553,11 @@ export class DashboardScenePageStateManager extends DashboardScenePageStateManag const scene = transformSaveModelToScene(rsp); + // we need to call and restore dashboard state on every reload that pulls a new dashboard version + if (config.featureToggles.preserveDashboardStateWhenNavigating && Boolean(options.uid)) { + restoreDashboardStateFromLocalStorage(scene); + } + this.setSceneCache(options.uid, scene); this.setState({ dashboard: scene, isLoading: false, options }); @@ -696,7 +714,11 @@ export class DashboardScenePageStateManagerV2 extends DashboardScenePageStateMan const rsp = await this.fetchDashboard(options); const fromCache = this.getSceneFromCache(options.uid); - if (fromCache && fromCache.state.version === rsp?.metadata.generation) { + if ( + fromCache && + fromCache.state.version === rsp?.metadata.generation && + fromCache.state.version === this.state.dashboard?.state.version + ) { this.setState({ isLoading: false }); return; } @@ -714,6 +736,11 @@ export class DashboardScenePageStateManagerV2 extends DashboardScenePageStateMan const scene = transformSaveModelSchemaV2ToScene(rsp); + // we need to call and restore dashboard state on every reload that pulls a new dashboard version + if (config.featureToggles.preserveDashboardStateWhenNavigating && Boolean(options.uid)) { + restoreDashboardStateFromLocalStorage(scene); + } + this.setSceneCache(options.uid, scene); this.setState({ dashboard: scene, isLoading: false, options }); diff --git a/public/app/features/dashboard-scene/serialization/transformSaveModelSchemaV2ToScene.ts b/public/app/features/dashboard-scene/serialization/transformSaveModelSchemaV2ToScene.ts index 0fa343a5c4e..99c858ee5df 100644 --- a/public/app/features/dashboard-scene/serialization/transformSaveModelSchemaV2ToScene.ts +++ b/public/app/features/dashboard-scene/serialization/transformSaveModelSchemaV2ToScene.ts @@ -59,7 +59,6 @@ import { registerDashboardMacro } from '../scene/DashboardMacro'; import { DashboardReloadBehavior } from '../scene/DashboardReloadBehavior'; import { DashboardScene } from '../scene/DashboardScene'; import { DashboardLayoutManager } from '../scene/types/DashboardLayoutManager'; -import { preserveDashboardSceneStateInLocalStorage } from '../utils/dashboardSessionState'; import { getIntervalsFromQueryString } from '../utils/utils'; import { SnapshotVariable } from './custom-variables/SnapshotVariable'; @@ -195,7 +194,6 @@ export function transformSaveModelSchemaV2ToScene(dto: DashboardWithAccessInfo { beforeAll(() => { @@ -18,35 +22,21 @@ describe('dashboardSessionState', () => { beforeEach(() => {}); describe('behavior', () => { - it('should do nothing for default home dashboard', () => { - const scene = buildTestScene(); - scene.setState({ uid: undefined }); - - const deactivate = scene.activate(); + it('should do nothing if no uid', () => { expect(window.sessionStorage.getItem(PRESERVED_SCENE_STATE_KEY)).toBeNull(); - deactivate(); + preserveDashboardSceneStateInLocalStorage({} as URLSearchParams, undefined); + expect(window.sessionStorage.getItem(PRESERVED_SCENE_STATE_KEY)).toBeNull(); }); - it('should do nothing if dashboard version is 0', () => { - const scene = buildTestScene(); - scene.setState({ version: 0 }); + it('should capture dashboard scene state and save it to session storage', () => { + const params = new URLSearchParams('from=now-6h&to=now&timezone=browser&var-customVar=a'); - const deactivate = scene.activate(); expect(window.sessionStorage.getItem(PRESERVED_SCENE_STATE_KEY)).toBeNull(); - deactivate(); - expect(window.sessionStorage.getItem(PRESERVED_SCENE_STATE_KEY)).toBeNull(); - }); + preserveDashboardSceneStateInLocalStorage(params, 'uid'); - it('should capture dashboard scene state and save it to session storage on deactivation', () => { - const scene = buildTestScene(); - - const deactivate = scene.activate(); - expect(window.sessionStorage.getItem(PRESERVED_SCENE_STATE_KEY)).toBeNull(); - - deactivate(); expect(window.sessionStorage.getItem(PRESERVED_SCENE_STATE_KEY)).toBe( '?from=now-6h&to=now&timezone=browser&var-customVar=a' ); @@ -71,6 +61,45 @@ describe('dashboardSessionState', () => { expect(timeRange?.state.to).toEqual('now'); }); + it('should use preserved state filters if current location has empty filters state', () => { + // we have a saved state with filters set + window.sessionStorage.setItem( + PRESERVED_SCENE_STATE_KEY, + '?var-customVar=b&var-nonApplicableVar=b&from=now-5m&to=now&timezone=browser&var-filters=cluster%7C%3D%7Cdev&var-filters=test' + ); + + // but the target location also has filters key with an empty state + locationService.replace({ + search: 'var-customVar=b&from=now-5m&to=now&timezone=browser&var-filters=', + }); + + // var-filters must also be set on the scene otherwise restore fn will drop the filter url key + const scene = buildTestScene({ + templating: { + list: [ + { + multi: true, + name: 'customVar', + query: 'a,b,c', + type: 'custom', + }, + { + name: 'filters', + type: 'adhoc', + }, + ], + }, + }); + + restoreDashboardStateFromLocalStorage(scene); + + expect(locationService.getLocation().search).toBe( + '?var-customVar=b&from=now-5m&to=now&timezone=browser&var-filters=cluster%7C%3D%7Cdev&var-filters=test' + ); + + jest.clearAllMocks(); + }); + it('should remove query params that are not applicable on a target dashboard', () => { window.sessionStorage.setItem( PRESERVED_SCENE_STATE_KEY, @@ -95,23 +124,10 @@ describe('dashboardSessionState', () => { expect(locationService.getLocation().search).toBe('?var-customVar=b&from=now-6h&to=now&timezone=browser'); }); - - it('should not restore state if dashboard version is 0', () => { - window.sessionStorage.setItem( - PRESERVED_SCENE_STATE_KEY, - '?var-customVarNotOnDB=b&from=now-5m&to=now&timezone=browser' - ); - const scene = buildTestScene(); - scene.setState({ version: 0 }); - - restoreDashboardStateFromLocalStorage(scene); - - expect(locationService.getLocation().search).toBe('?var-customVar=b&from=now-6h&to=now&timezone=browser'); - }); }); }); -function buildTestScene() { +function buildTestScene(overrides?: Partial) { const testDashboard: DashboardDataDTO = { annotations: { list: [] }, editable: true, @@ -142,6 +158,7 @@ function buildTestScene() { uid: 'edhmd9stpd6o0a', version: 24, weekStart: '', + ...overrides, }; const scene = transformSaveModelToScene({ dashboard: testDashboard, meta: {} }); diff --git a/public/app/features/dashboard-scene/utils/dashboardSessionState.ts b/public/app/features/dashboard-scene/utils/dashboardSessionState.ts index c31a29843ca..440965105c3 100644 --- a/public/app/features/dashboard-scene/utils/dashboardSessionState.ts +++ b/public/app/features/dashboard-scene/utils/dashboardSessionState.ts @@ -1,30 +1,36 @@ import { UrlQueryMap, urlUtil } from '@grafana/data'; import { config, locationService } from '@grafana/runtime'; -import { UrlSyncManager } from '@grafana/scenes'; import { DashboardScene } from '../scene/DashboardScene'; export const PRESERVED_SCENE_STATE_KEY = `grafana.dashboard.preservedUrlFiltersState`; +// TODO - deal with all this complexity, more details here https://github.com/grafana/grafana/pull/104780 export function restoreDashboardStateFromLocalStorage(dashboard: DashboardScene) { - if (!dashboard.state.version) { - return; - } - const preservedUrlState = window.sessionStorage.getItem(PRESERVED_SCENE_STATE_KEY); if (preservedUrlState) { const preservedQueryParams = new URLSearchParams(preservedUrlState); const currentQueryParams = locationService.getSearch(); + const cleanedQueryParams = new URLSearchParams(); // iterate over preserved query params and append them to current query params if they don't already exist preservedQueryParams.forEach((value, key) => { - if (!currentQueryParams.has(key)) { + // if somehow there are keys set with no values, we append new key-value pairs, + // but need to clean empty ones after this loop so we don't lose any values + if (!currentQueryParams.has(key) || currentQueryParams.get(key) === '') { currentQueryParams.append(key, value); } }); - for (const key of Array.from(currentQueryParams.keys())) { + // remove empty values + currentQueryParams.forEach((value, key) => { + if (value !== '') { + cleanedQueryParams.append(key, value); + } + }); + + for (const key of Array.from(cleanedQueryParams.keys())) { // preserve non-variable query params, i.e. time range if (!key.startsWith('var-')) { continue; @@ -32,11 +38,11 @@ export function restoreDashboardStateFromLocalStorage(dashboard: DashboardScene) // remove params for variables that are not present on the target dashboard if (!dashboard.state.$variables?.getByName(key.replace('var-', ''))) { - currentQueryParams.delete(key); + cleanedQueryParams.delete(key); } } - const finalParams = currentQueryParams.toString(); + const finalParams = cleanedQueryParams.toString(); if (finalParams) { locationService.replace({ search: finalParams }); } @@ -46,32 +52,35 @@ export function restoreDashboardStateFromLocalStorage(dashboard: DashboardScene) /** * Scenes behavior that will capture currently selected variables and time range and save them to local storage, so that they can be applied when the next dashboard is loaded. */ -export function preserveDashboardSceneStateInLocalStorage(scene: DashboardScene) { +export function preserveDashboardSceneStateInLocalStorage(search: URLSearchParams, uid?: string) { if (!config.featureToggles.preserveDashboardStateWhenNavigating) { return; } - return () => { - // Skipping saving state for default home dashboard - if (!scene.state.uid || !scene.state.version) { - return; - } + // Skipping saving state for default home dashboard + if (!uid) { + return; + } - const urlStates: UrlQueryMap = Object.fromEntries( - Object.entries(new UrlSyncManager().getUrlState(scene)).filter( - ([key]) => key.startsWith('var-') || key === 'from' || key === 'to' || key === 'timezone' - ) - ); + const queryParams: Record = {}; + search.forEach((value, key) => { + queryParams[key] = value; + }); - const nonEmptyUrlStates = Object.fromEntries( - Object.entries(urlStates).filter(([key, value]) => !(Array.isArray(value) && value.length === 0)) - ); + const urlStates: UrlQueryMap = Object.fromEntries( + Object.entries(queryParams).filter( + ([key]) => key.startsWith('var-') || key === 'from' || key === 'to' || key === 'timezone' + ) + ); - // If there's anything to preserve, save it to local storage - if (Object.keys(nonEmptyUrlStates).length > 0) { - window.sessionStorage.setItem(PRESERVED_SCENE_STATE_KEY, urlUtil.renderUrl('', nonEmptyUrlStates)); - } else { - window.sessionStorage.removeItem(PRESERVED_SCENE_STATE_KEY); - } - }; + const nonEmptyUrlStates = Object.fromEntries( + Object.entries(urlStates).filter(([key, value]) => !(Array.isArray(value) && value.length === 0)) + ); + + // If there's anything to preserve, save it to local storage + if (Object.keys(nonEmptyUrlStates).length > 0) { + window.sessionStorage.setItem(PRESERVED_SCENE_STATE_KEY, urlUtil.renderUrl('', nonEmptyUrlStates)); + } else { + window.sessionStorage.removeItem(PRESERVED_SCENE_STATE_KEY); + } }