From d5ea023dd473ec925c492e9881243f78d3b17dcd Mon Sep 17 00:00:00 2001 From: Nar Saynorath Date: Thu, 22 May 2025 11:03:54 -0400 Subject: [PATCH 1/4] feat(widget-builder): Cache builder state between dataset changes Either restore the old state if it exists, or push the dataset change through the original way the widget builder hook works. --- .../components/datasetSelector.tsx | 21 +- .../hooks/useCacheBuilderState.spec.tsx | 195 ++++++++++++++++++ .../hooks/useCacheBuilderState.tsx | 70 +++++++ 3 files changed, 278 insertions(+), 8 deletions(-) create mode 100644 static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.spec.tsx create mode 100644 static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.tsx diff --git a/static/app/views/dashboards/widgetBuilder/components/datasetSelector.tsx b/static/app/views/dashboards/widgetBuilder/components/datasetSelector.tsx index 310a370c3478e7..bb90352d19b791 100644 --- a/static/app/views/dashboards/widgetBuilder/components/datasetSelector.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/datasetSelector.tsx @@ -11,15 +11,16 @@ import useOrganization from 'sentry/utils/useOrganization'; import {WidgetType} from 'sentry/views/dashboards/types'; import {SectionHeader} from 'sentry/views/dashboards/widgetBuilder/components/common/sectionHeader'; import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import {useCacheBuilderState} from 'sentry/views/dashboards/widgetBuilder/hooks/useCacheBuilderState'; import useDashboardWidgetSource from 'sentry/views/dashboards/widgetBuilder/hooks/useDashboardWidgetSource'; import useIsEditingWidget from 'sentry/views/dashboards/widgetBuilder/hooks/useIsEditingWidget'; -import {BuilderStateAction} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; function WidgetBuilderDatasetSelector() { const organization = useOrganization(); - const {state, dispatch} = useWidgetBuilderContext(); + const {state} = useWidgetBuilderContext(); const source = useDashboardWidgetSource(); const isEditing = useIsEditingWidget(); + const {cacheBuilderState, restoreOrSetBuilderState} = useCacheBuilderState(); const datasetChoices: Array> = []; datasetChoices.push([WidgetType.ERRORS, t('Errors')]); @@ -48,17 +49,21 @@ function WidgetBuilderDatasetSelector() { label={t('Dataset')} value={state.dataset ?? WidgetType.ERRORS} choices={datasetChoices} - onChange={(newValue: WidgetType) => { - dispatch({ - type: BuilderStateAction.SET_DATASET, - payload: newValue, - }); + onChange={(newDataset: WidgetType) => { + // Set the current dataset state in local storage for recovery + // when the user navigates back to this dataset + cacheBuilderState(state.dataset ?? WidgetType.ERRORS); + + // Restore the builder state for the new dataset + // or set the dataset if there is no cached state + restoreOrSetBuilderState(newDataset); + trackAnalytics('dashboards_views.widget_builder.change', { from: source, widget_type: state.dataset ?? '', builder_version: WidgetBuilderVersion.SLIDEOUT, field: 'dataSet', - value: newValue, + value: newDataset, new_widget: !isEditing, organization, }); diff --git a/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.spec.tsx b/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.spec.tsx new file mode 100644 index 00000000000000..d3c60cc172ccac --- /dev/null +++ b/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.spec.tsx @@ -0,0 +1,195 @@ +import {LocationFixture} from 'sentry-fixture/locationFixture'; + +import {renderHook} from 'sentry-test/reactTestingLibrary'; + +import {useLocation} from 'sentry/utils/useLocation'; +import {DisplayType, WidgetType} from 'sentry/views/dashboards/types'; +import { + useWidgetBuilderContext, + WidgetBuilderProvider, +} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import { + BuilderStateAction, + type WidgetBuilderState, +} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; +import {convertBuilderStateToWidget} from 'sentry/views/dashboards/widgetBuilder/utils/convertBuilderStateToWidget'; + +import {useCacheBuilderState} from './useCacheBuilderState'; + +jest.mock('sentry/utils/useNavigate', () => ({ + useNavigate: jest.fn(), +})); +jest.mock('sentry/utils/useLocation'); + +jest.mock('sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext', () => ({ + useWidgetBuilderContext: jest.fn(), + WidgetBuilderProvider: jest.requireActual( + 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext' + ).WidgetBuilderProvider, +})); + +const mockUseWidgetBuilderContext = jest.mocked(useWidgetBuilderContext); +const mockUseLocation = jest.mocked(useLocation); + +function Wrapper({children}: {children: React.ReactNode}) { + return {children}; +} + +describe('useCacheBuilderState', () => { + let mockLocalStorage: Record; + + beforeEach(() => { + mockLocalStorage = {}; + mockUseWidgetBuilderContext.mockReturnValue({ + state: {}, + dispatch: jest.fn(), + }); + mockUseLocation.mockReturnValue(LocationFixture()); + + Storage.prototype.getItem = jest.fn(key => mockLocalStorage[key] ?? null); + Storage.prototype.setItem = jest.fn((key, value) => { + mockLocalStorage[key] = value; + }); + Storage.prototype.removeItem = jest.fn(key => { + delete mockLocalStorage[key]; + }); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('caches builder state to localStorage', () => { + const cachedWidget: WidgetBuilderState = { + dataset: WidgetType.ERRORS, + displayType: DisplayType.LINE, + yAxis: [ + { + function: ['count', '', undefined, undefined], + kind: 'function', + }, + ], + query: ['this is a test query'], + }; + mockUseWidgetBuilderContext.mockReturnValue({ + state: cachedWidget, + dispatch: jest.fn(), + }); + + const {result} = renderHook(() => useCacheBuilderState(), { + wrapper: Wrapper, + }); + + result.current.cacheBuilderState(WidgetType.ERRORS); + + // Verify state was saved to localStorage + expect(localStorage.setItem).toHaveBeenCalledWith( + 'dashboards:widget-builder:dataset:error-events', + JSON.stringify(convertBuilderStateToWidget(cachedWidget)) + ); + + result.current.restoreOrSetBuilderState(WidgetType.ERRORS); + + expect(localStorage.getItem).toHaveBeenCalledWith( + 'dashboards:widget-builder:dataset:error-events' + ); + }); + + it('restores builder state from localStorage when available', () => { + const cachedWidget: WidgetBuilderState = { + dataset: WidgetType.ERRORS, + displayType: DisplayType.LINE, + yAxis: [ + { + function: ['count', '', undefined, undefined], + kind: 'function', + }, + ], + query: ['this is a test query'], + }; + const currentWidget: WidgetBuilderState = { + dataset: WidgetType.ISSUE, + displayType: DisplayType.TABLE, + query: ['issue.id:123'], + fields: [ + { + field: 'issue', + kind: 'field', + }, + ], + }; + const mockDispatch = jest.fn(); + mockUseWidgetBuilderContext.mockReturnValue({ + state: currentWidget, + dispatch: mockDispatch, + }); + // Add cached widget to the localStorage + localStorage.setItem( + 'dashboards:widget-builder:dataset:error-events', + JSON.stringify(convertBuilderStateToWidget(cachedWidget)) + ); + + const {result} = renderHook(() => useCacheBuilderState(), { + wrapper: Wrapper, + }); + + // Call the restore helper on the cached dataset + result.current.restoreOrSetBuilderState(WidgetType.ERRORS); + + expect(mockDispatch).toHaveBeenCalledWith({ + type: BuilderStateAction.SET_STATE, + + // the yAxis gets converted to a string when used with this payload + payload: expect.objectContaining({...cachedWidget, yAxis: ['count()']}), + }); + }); + + it('plainly sets the new dataset when no cached state exists', () => { + const cachedWidget: WidgetBuilderState = { + dataset: WidgetType.ERRORS, + displayType: DisplayType.LINE, + yAxis: [ + { + function: ['count', '', undefined, undefined], + kind: 'function', + }, + ], + query: ['this is a test query'], + }; + const currentWidget: WidgetBuilderState = { + dataset: WidgetType.ISSUE, + displayType: DisplayType.TABLE, + query: ['issue.id:123'], + fields: [ + { + field: 'issue', + kind: 'field', + }, + ], + }; + const mockDispatch = jest.fn(); + mockUseWidgetBuilderContext.mockReturnValue({ + state: currentWidget, + dispatch: mockDispatch, + }); + // Add cached widget to the localStorage, this will not be the one + // used in the test to test that a cache miss falls back to the plain + // dataset change + localStorage.setItem( + 'dashboards:widget-builder:dataset:error-events', + JSON.stringify(convertBuilderStateToWidget(cachedWidget)) + ); + + const {result} = renderHook(() => useCacheBuilderState(), { + wrapper: Wrapper, + }); + + // Call the restore helper on the cached dataset + result.current.restoreOrSetBuilderState(WidgetType.TRANSACTIONS); + + expect(mockDispatch).toHaveBeenCalledWith({ + type: BuilderStateAction.SET_DATASET, + payload: WidgetType.TRANSACTIONS, + }); + }); +}); diff --git a/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.tsx b/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.tsx new file mode 100644 index 00000000000000..273e0483750b1d --- /dev/null +++ b/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.tsx @@ -0,0 +1,70 @@ +import {useCallback, useEffect} from 'react'; + +import type {WidgetType} from 'sentry/views/dashboards/types'; +import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import {BuilderStateAction} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; +import {convertBuilderStateToWidget} from 'sentry/views/dashboards/widgetBuilder/utils/convertBuilderStateToWidget'; +import {convertWidgetToBuilderStateParams} from 'sentry/views/dashboards/widgetBuilder/utils/convertWidgetToBuilderStateParams'; + +const WIDGET_BUILDER_DATASET_STATE_KEY = 'dashboards:widget-builder:dataset'; + +function cleanUpDatasetState() { + for (let i = 0; i < localStorage.length; i++) { + const key = localStorage.key(i); + if (key?.startsWith(WIDGET_BUILDER_DATASET_STATE_KEY)) { + localStorage.removeItem(key); + } + } +} + +/** + * This hook is used to cache the builder state for the given dataset + * and restore it when the user navigates back to the same dataset. + */ +export function useCacheBuilderState() { + const {state, dispatch} = useWidgetBuilderContext(); + + useEffect(() => { + return cleanUpDatasetState; + }, []); + + const cacheBuilderState = useCallback( + (dataset: WidgetType) => { + localStorage.setItem( + `${WIDGET_BUILDER_DATASET_STATE_KEY}:${dataset}`, + JSON.stringify(convertBuilderStateToWidget(state)) + ); + }, + [state] + ); + + // Checks if there is a cached builder state for the given dataset + // and restores it if it exists. Otherwise, it sets the dataset. + const restoreOrSetBuilderState = useCallback( + (nextDataset: WidgetType) => { + const previousDatasetState = localStorage.getItem( + `${WIDGET_BUILDER_DATASET_STATE_KEY}:${nextDataset}` + ); + if (previousDatasetState) { + const builderState = convertWidgetToBuilderStateParams( + JSON.parse(previousDatasetState) + ); + dispatch({ + type: BuilderStateAction.SET_STATE, + payload: builderState, + }); + } else { + dispatch({ + type: BuilderStateAction.SET_DATASET, + payload: nextDataset, + }); + } + }, + [dispatch] + ); + + return { + cacheBuilderState, + restoreOrSetBuilderState, + }; +} From 379772397db2decd07a0291fa651773b20db7d97 Mon Sep 17 00:00:00 2001 From: Nar Saynorath Date: Thu, 22 May 2025 13:19:24 -0400 Subject: [PATCH 2/4] Ignore title, that should not change from a cached value --- .../widgetBuilder/hooks/useCacheBuilderState.spec.tsx | 8 +++++++- .../widgetBuilder/hooks/useCacheBuilderState.tsx | 4 ++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.spec.tsx b/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.spec.tsx index d3c60cc172ccac..f045a6c80d4d76 100644 --- a/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.spec.tsx +++ b/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.spec.tsx @@ -97,6 +97,7 @@ describe('useCacheBuilderState', () => { it('restores builder state from localStorage when available', () => { const cachedWidget: WidgetBuilderState = { + title: 'error widget title', dataset: WidgetType.ERRORS, displayType: DisplayType.LINE, yAxis: [ @@ -108,6 +109,7 @@ describe('useCacheBuilderState', () => { query: ['this is a test query'], }; const currentWidget: WidgetBuilderState = { + title: 'issue widget title', dataset: WidgetType.ISSUE, displayType: DisplayType.TABLE, query: ['issue.id:123'], @@ -140,7 +142,11 @@ describe('useCacheBuilderState', () => { type: BuilderStateAction.SET_STATE, // the yAxis gets converted to a string when used with this payload - payload: expect.objectContaining({...cachedWidget, yAxis: ['count()']}), + payload: expect.objectContaining({ + ...cachedWidget, + yAxis: ['count()'], + title: 'issue widget title', // The title was not overridden + }), }); }); diff --git a/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.tsx b/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.tsx index 273e0483750b1d..484777715288e6 100644 --- a/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.tsx +++ b/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.tsx @@ -51,7 +51,7 @@ export function useCacheBuilderState() { ); dispatch({ type: BuilderStateAction.SET_STATE, - payload: builderState, + payload: {...builderState, title: state.title}, }); } else { dispatch({ @@ -60,7 +60,7 @@ export function useCacheBuilderState() { }); } }, - [dispatch] + [dispatch, state.title] ); return { From 42c38a6a3c95db6949ad4a102278d847aaec60a0 Mon Sep 17 00:00:00 2001 From: Nar Saynorath Date: Thu, 22 May 2025 13:20:38 -0400 Subject: [PATCH 3/4] Ignore description too --- .../widgetBuilder/hooks/useCacheBuilderState.spec.tsx | 3 +++ .../dashboards/widgetBuilder/hooks/useCacheBuilderState.tsx | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.spec.tsx b/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.spec.tsx index f045a6c80d4d76..43a0bb07b73b15 100644 --- a/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.spec.tsx +++ b/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.spec.tsx @@ -98,6 +98,7 @@ describe('useCacheBuilderState', () => { it('restores builder state from localStorage when available', () => { const cachedWidget: WidgetBuilderState = { title: 'error widget title', + description: 'error widget description', dataset: WidgetType.ERRORS, displayType: DisplayType.LINE, yAxis: [ @@ -110,6 +111,7 @@ describe('useCacheBuilderState', () => { }; const currentWidget: WidgetBuilderState = { title: 'issue widget title', + description: 'issue widget description', dataset: WidgetType.ISSUE, displayType: DisplayType.TABLE, query: ['issue.id:123'], @@ -146,6 +148,7 @@ describe('useCacheBuilderState', () => { ...cachedWidget, yAxis: ['count()'], title: 'issue widget title', // The title was not overridden + description: 'issue widget description', // The description was not overridden }), }); }); diff --git a/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.tsx b/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.tsx index 484777715288e6..f49ef0c5e0b551 100644 --- a/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.tsx +++ b/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.tsx @@ -51,7 +51,7 @@ export function useCacheBuilderState() { ); dispatch({ type: BuilderStateAction.SET_STATE, - payload: {...builderState, title: state.title}, + payload: {...builderState, title: state.title, description: state.description}, }); } else { dispatch({ @@ -60,7 +60,7 @@ export function useCacheBuilderState() { }); } }, - [dispatch, state.title] + [dispatch, state.title, state.description] ); return { From 53b03f7d9312a13fabb2463ef34391b18a6fd7d4 Mon Sep 17 00:00:00 2001 From: Nar Saynorath Date: Fri, 23 May 2025 15:37:37 -0400 Subject: [PATCH 4/4] use session storage and forcibly clean up cached state --- .../hooks/useCacheBuilderState.tsx | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.tsx b/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.tsx index f49ef0c5e0b551..66b1d238033c5d 100644 --- a/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.tsx +++ b/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.tsx @@ -1,5 +1,6 @@ import {useCallback, useEffect} from 'react'; +import createStorage from 'sentry/utils/createStorage'; import type {WidgetType} from 'sentry/views/dashboards/types'; import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {BuilderStateAction} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; @@ -8,11 +9,13 @@ import {convertWidgetToBuilderStateParams} from 'sentry/views/dashboards/widgetB const WIDGET_BUILDER_DATASET_STATE_KEY = 'dashboards:widget-builder:dataset'; +const STORAGE = createStorage(() => window.sessionStorage); + function cleanUpDatasetState() { - for (let i = 0; i < localStorage.length; i++) { - const key = localStorage.key(i); + for (let i = 0; i < STORAGE.length; i++) { + const key = STORAGE.key(i); if (key?.startsWith(WIDGET_BUILDER_DATASET_STATE_KEY)) { - localStorage.removeItem(key); + STORAGE.removeItem(key); } } } @@ -25,12 +28,16 @@ export function useCacheBuilderState() { const {state, dispatch} = useWidgetBuilderContext(); useEffect(() => { + // Remove all cached dataset states when the component mounts + // to prevent stale data from being used. + cleanUpDatasetState(); + return cleanUpDatasetState; }, []); const cacheBuilderState = useCallback( (dataset: WidgetType) => { - localStorage.setItem( + STORAGE.setItem( `${WIDGET_BUILDER_DATASET_STATE_KEY}:${dataset}`, JSON.stringify(convertBuilderStateToWidget(state)) ); @@ -42,7 +49,7 @@ export function useCacheBuilderState() { // and restores it if it exists. Otherwise, it sets the dataset. const restoreOrSetBuilderState = useCallback( (nextDataset: WidgetType) => { - const previousDatasetState = localStorage.getItem( + const previousDatasetState = STORAGE.getItem( `${WIDGET_BUILDER_DATASET_STATE_KEY}:${nextDataset}` ); if (previousDatasetState) {