diff --git a/public/app/features/dashboard-scene/pages/DashboardScenePage.tsx b/public/app/features/dashboard-scene/pages/DashboardScenePage.tsx index b13ca336072..9e883816200 100644 --- a/public/app/features/dashboard-scene/pages/DashboardScenePage.tsx +++ b/public/app/features/dashboard-scene/pages/DashboardScenePage.tsx @@ -15,7 +15,6 @@ 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'; @@ -48,7 +47,6 @@ 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 c32616a0b9c..0721d2f32cf 100644 --- a/public/app/features/dashboard-scene/pages/DashboardScenePageStateManager.test.ts +++ b/public/app/features/dashboard-scene/pages/DashboardScenePageStateManager.test.ts @@ -719,57 +719,6 @@ 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( @@ -994,44 +943,6 @@ 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 a7fe4ce73fc..438c8d5cd73 100644 --- a/public/app/features/dashboard-scene/pages/DashboardScenePageStateManager.ts +++ b/public/app/features/dashboard-scene/pages/DashboardScenePageStateManager.ts @@ -522,20 +522,7 @@ export class DashboardScenePageStateManager extends DashboardScenePageStateManag const rsp = await this.fetchDashboard(options); const fromCache = this.getSceneFromCache(options.uid); - // 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 - ) { + if (fromCache && fromCache.state.version === rsp?.dashboard.version) { this.setState({ isLoading: false }); return; } @@ -553,11 +540,6 @@ 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 }); @@ -714,11 +696,7 @@ 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 && - fromCache.state.version === this.state.dashboard?.state.version - ) { + if (fromCache && fromCache.state.version === rsp?.metadata.generation) { this.setState({ isLoading: false }); return; } @@ -736,11 +714,6 @@ 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 dcc683771f0..9fe1904ba69 100644 --- a/public/app/features/dashboard-scene/serialization/transformSaveModelSchemaV2ToScene.ts +++ b/public/app/features/dashboard-scene/serialization/transformSaveModelSchemaV2ToScene.ts @@ -60,6 +60,7 @@ 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,6 +196,7 @@ export function transformSaveModelSchemaV2ToScene(dto: DashboardWithAccessInfo { beforeAll(() => { @@ -22,21 +18,35 @@ describe('dashboardSessionState', () => { beforeEach(() => {}); describe('behavior', () => { - it('should do nothing if no uid', () => { + it('should do nothing for default home dashboard', () => { + const scene = buildTestScene(); + scene.setState({ uid: undefined }); + + const deactivate = scene.activate(); expect(window.sessionStorage.getItem(PRESERVED_SCENE_STATE_KEY)).toBeNull(); - preserveDashboardSceneStateInLocalStorage({} as URLSearchParams, undefined); - + deactivate(); expect(window.sessionStorage.getItem(PRESERVED_SCENE_STATE_KEY)).toBeNull(); }); - 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'); + it('should do nothing if dashboard version is 0', () => { + const scene = buildTestScene(); + scene.setState({ version: 0 }); + const deactivate = scene.activate(); expect(window.sessionStorage.getItem(PRESERVED_SCENE_STATE_KEY)).toBeNull(); - preserveDashboardSceneStateInLocalStorage(params, 'uid'); + deactivate(); + expect(window.sessionStorage.getItem(PRESERVED_SCENE_STATE_KEY)).toBeNull(); + }); + 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' ); @@ -61,45 +71,6 @@ 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, @@ -124,10 +95,23 @@ 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(overrides?: Partial) { +function buildTestScene() { const testDashboard: DashboardDataDTO = { annotations: { list: [] }, editable: true, @@ -158,7 +142,6 @@ function buildTestScene(overrides?: Partial) { 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 440965105c3..c31a29843ca 100644 --- a/public/app/features/dashboard-scene/utils/dashboardSessionState.ts +++ b/public/app/features/dashboard-scene/utils/dashboardSessionState.ts @@ -1,36 +1,30 @@ 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 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) === '') { + if (!currentQueryParams.has(key)) { currentQueryParams.append(key, value); } }); - // remove empty values - currentQueryParams.forEach((value, key) => { - if (value !== '') { - cleanedQueryParams.append(key, value); - } - }); - - for (const key of Array.from(cleanedQueryParams.keys())) { + for (const key of Array.from(currentQueryParams.keys())) { // preserve non-variable query params, i.e. time range if (!key.startsWith('var-')) { continue; @@ -38,11 +32,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-', ''))) { - cleanedQueryParams.delete(key); + currentQueryParams.delete(key); } } - const finalParams = cleanedQueryParams.toString(); + const finalParams = currentQueryParams.toString(); if (finalParams) { locationService.replace({ search: finalParams }); } @@ -52,35 +46,32 @@ 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(search: URLSearchParams, uid?: string) { +export function preserveDashboardSceneStateInLocalStorage(scene: DashboardScene) { if (!config.featureToggles.preserveDashboardStateWhenNavigating) { return; } - // Skipping saving state for default home dashboard - if (!uid) { - return; - } + return () => { + // Skipping saving state for default home dashboard + if (!scene.state.uid || !scene.state.version) { + return; + } - const queryParams: Record = {}; - search.forEach((value, key) => { - queryParams[key] = value; - }); + const urlStates: UrlQueryMap = Object.fromEntries( + Object.entries(new UrlSyncManager().getUrlState(scene)).filter( + ([key]) => key.startsWith('var-') || key === 'from' || key === 'to' || key === 'timezone' + ) + ); - const urlStates: UrlQueryMap = Object.fromEntries( - Object.entries(queryParams).filter( - ([key]) => key.startsWith('var-') || key === 'from' || key === 'to' || key === 'timezone' - ) - ); + const nonEmptyUrlStates = Object.fromEntries( + Object.entries(urlStates).filter(([key, value]) => !(Array.isArray(value) && value.length === 0)) + ); - 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); - } + // 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); + } + }; }