From 30be6256da4c46e63ca47f2a5b6801596665170d Mon Sep 17 00:00:00 2001 From: Scott Cooper Date: Wed, 8 Jul 2026 10:21:20 -0700 Subject: [PATCH 1/3] fix(charts): Preserve ECharts zoom state Dragging to zoom can re-init the hidden ECharts toolbox and replay stale dataZoom events, which makes issue detail charts push the wrong time range after rerenders. Centralize the merge props from the dashboards fix and reuse them from both ChartZoom implementations. Keep the inside dataZoom model around for synced charts while disabling the hidden toolbox when navigation is off. Adds a small hook test for rerenders and disabled zoom behavior. Co-Authored-By: Codex --- static/app/components/charts/chartZoom.tsx | 10 +- .../app/components/charts/chartZoomConfig.ts | 20 ++ .../components/charts/useChartZoom.spec.tsx | 109 ++++++ static/app/components/charts/useChartZoom.tsx | 319 +++++++++++------- static/app/components/charts/utils.tsx | 16 +- .../timeSeriesWidgetVisualization.tsx | 16 - static/app/views/issueDetails/eventGraph.tsx | 8 +- 7 files changed, 344 insertions(+), 154 deletions(-) create mode 100644 static/app/components/charts/chartZoomConfig.ts create mode 100644 static/app/components/charts/useChartZoom.spec.tsx diff --git a/static/app/components/charts/chartZoom.tsx b/static/app/components/charts/chartZoom.tsx index bf7fb3b61ce9..ea723f342a1c 100644 --- a/static/app/components/charts/chartZoom.tsx +++ b/static/app/components/charts/chartZoom.tsx @@ -9,6 +9,7 @@ import type {Location} from 'history'; import moment, {type MomentInput} from 'moment-timezone'; import * as qs from 'query-string'; +import {CHART_ZOOM_MERGE_OPTIONS} from 'sentry/components/charts/chartZoomConfig'; import {DataZoomInside} from 'sentry/components/charts/components/dataZoomInside'; import {ToolBox} from 'sentry/components/charts/components/toolBox'; import {activateZoomAreaSelect} from 'sentry/components/charts/utils'; @@ -46,6 +47,8 @@ export interface ZoomRenderProps extends Pick { dataZoom?: DataZoomComponentOption[]; end?: Date; isGroupedByDate?: boolean; + notMerge?: boolean; + replaceMerge?: string[]; showTimeInTooltip?: boolean; start?: Date; toolBox?: ToolboxComponentOption; @@ -326,13 +329,13 @@ class ChartZoom extends Component { utc, start, end, - isGroupedByDate: true, + ...CHART_ZOOM_MERGE_OPTIONS, ...props, }); } const renderProps = { - // Zooming only works when grouped by date - isGroupedByDate: true, + ...props, + ...CHART_ZOOM_MERGE_OPTIONS, onChartReady: this.handleChartReady, utc, start, @@ -360,7 +363,6 @@ class ChartZoom extends Component { onDataZoom: this.handleDataZoom, onFinished: this.handleChartFinished, onRestore: this.handleZoomRestore, - ...props, }; return children(renderProps); diff --git a/static/app/components/charts/chartZoomConfig.ts b/static/app/components/charts/chartZoomConfig.ts new file mode 100644 index 000000000000..7a14193f83d0 --- /dev/null +++ b/static/app/components/charts/chartZoomConfig.ts @@ -0,0 +1,20 @@ +export const CHART_ZOOM_MERGE_OPTIONS = { + // Zooming only works when grouped by date. + isGroupedByDate: true, + // `notMerge` should always be `false`. i.e., ECharts should be + // allowed to _merge_ the incoming options when they change. Note + // `replaceMerge` below which ensures that the critical components + // like the series and the axes are merged using the "replace" + // algorithm, not the "normal" algorithm. + // + // Under `notMerge`, every data refresh does a full ECharts re-init that + // destroys and re-creates the toolbox dataZoom "select" component. In + // ECharts 6.1 that rebuild re-emits a stale `dataZoom` event, so a + // single drag-to-zoom cascades into repeated refetches that settle on + // the wrong time range. See apache/echarts#21661. + // + // To guard against this, we allow ECharts to preserve the + // configuration of the toolbox, which prevents these stale fires. + notMerge: false, + replaceMerge: ['series', 'xAxis', 'yAxis'], +}; diff --git a/static/app/components/charts/useChartZoom.spec.tsx b/static/app/components/charts/useChartZoom.spec.tsx new file mode 100644 index 000000000000..a4bf0e39b17e --- /dev/null +++ b/static/app/components/charts/useChartZoom.spec.tsx @@ -0,0 +1,109 @@ +import {act, renderHookWithProviders} from 'sentry-test/reactTestingLibrary'; + +import type {EChartDataZoomHandler} from 'sentry/types/echarts'; + +import {useChartZoom} from './useChartZoom'; + +const START_VALUE = Date.UTC(2026, 6, 3, 11, 1, 30); +const END_VALUE = Date.UTC(2026, 6, 3, 16, 54, 30); + +function dataZoomPayload( + startValue = START_VALUE, + endValue = END_VALUE +): Parameters[0] { + return { + type: 'datazoom', + start: 0, + end: 100, + batch: [{startValue, endValue}], + } as Parameters[0]; +} + +describe('useChartZoom', () => { + it('keeps zoom merge props and the inside dataZoom model across disabled rerenders', () => { + const {result, rerender} = renderHookWithProviders(useChartZoom, { + initialProps: {saveOnZoom: true}, + }); + + expect(result.current).toMatchObject({ + isGroupedByDate: true, + notMerge: false, + replaceMerge: ['series', 'xAxis', 'yAxis'], + }); + expect(result.current.dataZoom).toEqual([ + expect.objectContaining({ + id: 'useChartZoom-inside', + type: 'inside', + }), + ]); + expect(result.current.toolBox).toEqual( + expect.objectContaining({ + id: 'useChartZoom-toolbox', + }) + ); + + rerender({saveOnZoom: true, disabled: true}); + + expect(result.current).toMatchObject({ + isGroupedByDate: true, + notMerge: false, + replaceMerge: ['series', 'xAxis', 'yAxis'], + }); + expect(result.current.dataZoom).toEqual([ + expect.objectContaining({ + id: 'useChartZoom-inside', + type: 'inside', + }), + ]); + expect(result.current.toolBox).toEqual({}); + }); + + it('updates query params from a zoom event and no-ops while disabled', () => { + const {result, rerender, router} = renderHookWithProviders(useChartZoom, { + initialProps: { + usePageDate: true, + }, + initialRouterConfig: { + location: { + pathname: '/issues/1/', + query: {project: '11276', statsPeriod: '7d'}, + }, + }, + }); + + act(() => { + result.current.onDataZoom(dataZoomPayload(), {} as any); + }); + + expect(router.location.query).toEqual( + expect.objectContaining({ + project: '11276', + start: '2026-07-03T11:01:00', + end: '2026-07-03T16:55:00', + }) + ); + expect(router.location.query.statsPeriod).toBeUndefined(); + + rerender({ + disabled: true, + usePageDate: true, + }); + + act(() => { + result.current.onDataZoom( + dataZoomPayload( + Date.UTC(2026, 6, 4, 11, 1, 30), + Date.UTC(2026, 6, 4, 16, 54, 30) + ), + {} as any + ); + }); + + expect(router.location.query).toEqual( + expect.objectContaining({ + start: '2026-07-03T11:01:00', + end: '2026-07-03T16:55:00', + }) + ); + }); +}); diff --git a/static/app/components/charts/useChartZoom.tsx b/static/app/components/charts/useChartZoom.tsx index 715429373c2a..9a950011561b 100644 --- a/static/app/components/charts/useChartZoom.tsx +++ b/static/app/components/charts/useChartZoom.tsx @@ -2,6 +2,7 @@ import {useCallback, useEffect, useMemo, useRef} from 'react'; import type {DataZoomComponentOption, ECharts, ToolboxComponentOption} from 'echarts'; import * as qs from 'query-string'; +import {CHART_ZOOM_MERGE_OPTIONS} from 'sentry/components/charts/chartZoomConfig'; import {DataZoomInside} from 'sentry/components/charts/components/dataZoomInside'; import {ToolBox} from 'sentry/components/charts/components/toolBox'; import {activateZoomAreaSelect} from 'sentry/components/charts/utils'; @@ -20,28 +21,85 @@ import {useNavigate} from 'sentry/utils/useNavigate'; type DateTimeUpdate = Parameters[0]; +type DataZoomRange = { + endValue: number; + startValue: number; +}; + +type DataZoomRangePayload = { + endValue?: number | null; + startValue?: number | null; +}; + +type DataZoomPayload = { + batch?: DataZoomRangePayload[]; +} & DataZoomRangePayload; + /** * Our api query params expects a specific date format */ const getQueryTime = (date: DateString | undefined) => date ? getUtcDateString(date) : null; +function getFormattedPeriod({period, start, end}: DateTimeUpdate) { + return { + period, + start: getQueryTime(start), + end: getQueryTime(end), + }; +} + +type FormattedPeriod = ReturnType; + +function hasZoomValues(payload: DataZoomRangePayload): payload is DataZoomRange { + return ( + payload.startValue !== null && + payload.startValue !== undefined && + payload.endValue !== null && + payload.endValue !== undefined + ); +} + +function getZoomRange(evt: Parameters[0]): DataZoomRange | null { + const payload = evt as DataZoomPayload; + // Toolbox brush selections report their selected x-axis range in the first + // batch item. Some restore/back actions report nullish values instead. + const zoomEvent = payload.batch?.[0] ?? payload; + + return hasZoomValues(zoomEvent) ? zoomEvent : null; +} + +function roundZoomRange({startValue, endValue}: DataZoomRange): DataZoomRange { + const roundedEndValue = Math.ceil(endValue / 60_000) * 60_000; + let roundedStartValue = Math.floor(startValue / 60_000) * 60_000; + + // Ensure the bounds have at least 1 minute resolution. + roundedStartValue = Math.min(roundedStartValue, roundedEndValue - 60_000); + + return { + startValue: roundedStartValue, + endValue: roundedEndValue, + }; +} + interface ZoomRenderProps { dataZoom: DataZoomComponentOption[]; isGroupedByDate: boolean; + notMerge: boolean; onChartReady: EChartChartReadyHandler; onDataZoom: EChartDataZoomHandler; onFinished: EChartFinishedHandler; + replaceMerge: string[]; toolBox: ToolboxComponentOption; } -interface Props { - children: (props: ZoomRenderProps) => React.ReactNode; +interface UseChartZoomOptions { /** - * Disables saving changes to the current period + * Disables toolbox zoom interactions and URL updates while preserving the + * inside dataZoom model for synced charts. */ disabled?: boolean; - onZoom?: (period: DateTimeUpdate) => void; + onZoom?: (period: FormattedPeriod) => void; /** * Use either `saveOnZoom` or `usePageDate` not both * Will persist zoom state to page filters @@ -50,7 +108,7 @@ interface Props { /** * Use either `saveOnZoom` or `usePageDate` not both * Persists zoom state to query params without updating page filters. - * Sets the pageStart and pageEnd query params + * Sets the start, end, and statsPeriod query params. */ usePageDate?: boolean; xAxisIndex?: number | number[]; @@ -59,29 +117,37 @@ interface Props { /** * Adds listeners to the document to allow for cancelling the zoom action */ -function useChartZoomCancel() { +function useChartZoomCancel(disabled?: boolean) { const chartInstance = useRef(null); - const handleKeyDown = useCallback((evt: KeyboardEvent) => { - if (!chartInstance.current) { - return; - } + const handleKeyDown = useCallback( + (evt: KeyboardEvent) => { + if (disabled || !chartInstance.current) { + return; + } - if (evt.key === 'Escape') { - evt.stopPropagation(); - // Mark the component as currently cancelling a zoom selection. This allows - // us to prevent "restore" handlers from running - // "restore" removes the current chart zoom selection - chartInstance.current.dispatchAction({ - type: 'restore', - }); - } - }, []); + if (evt.key === 'Escape') { + evt.stopPropagation(); + // Mark the component as currently cancelling a zoom selection. This allows + // us to prevent "restore" handlers from running + // "restore" removes the current chart zoom selection + chartInstance.current.dispatchAction({ + type: 'restore', + }); + } + }, + [disabled] + ); const handleMouseUp = useCallback(() => { document.body.removeEventListener('mouseup', handleMouseUp); - }, []); + document.body.removeEventListener('keydown', handleKeyDown, true); + }, [handleKeyDown]); const handleMouseDown = useCallback(() => { + if (disabled) { + return; + } + // Register `mouseup` and `keydown` listeners on mouse down // This ensures that there is only one live listener at a time // regardless of how many charts are rendered. NOTE: It's @@ -90,7 +156,7 @@ function useChartZoomCancel() { // chart is in. Those elements register their handlers _earlier_. document.body.addEventListener('mouseup', handleMouseUp); document.body.addEventListener('keydown', handleKeyDown, true); - }, [handleKeyDown, handleMouseUp]); + }, [disabled, handleKeyDown, handleMouseUp]); const handleChartReady = useCallback( chart => { @@ -101,19 +167,32 @@ function useChartZoomCancel() { chartInstance.current = chart; const chartDom = chart.getDom(); - chartDom.addEventListener('mousedown', handleMouseDown); + if (!disabled) { + chartDom.addEventListener('mousedown', handleMouseDown); + } }, - [handleMouseDown] + [disabled, handleMouseDown] ); useEffect(() => { + const chartDom = chartInstance.current?.getDom(); + + if (disabled) { + chartDom?.removeEventListener('mousedown', handleMouseDown); + document.body.removeEventListener('mouseup', handleMouseUp); + document.body.removeEventListener('keydown', handleKeyDown, true); + return; + } + + chartDom?.addEventListener('mousedown', handleMouseDown); + return () => { // Cleanup listeners on unmount document.body.removeEventListener('mouseup', handleMouseUp); - document.body.removeEventListener('keydown', handleKeyDown); - chartInstance.current?.getDom()?.removeEventListener('mousedown', handleMouseDown); + document.body.removeEventListener('keydown', handleKeyDown, true); + chartDom?.removeEventListener('mousedown', handleMouseDown); }; - }, [handleMouseDown, handleMouseUp, handleKeyDown]); + }, [disabled, handleKeyDown, handleMouseDown, handleMouseUp]); return {handleChartReady}; } @@ -123,47 +202,24 @@ function useChartZoomCancel() { * the props that would be passed to the `BaseChart` as zoomRenderProps. */ export function useChartZoom({ + disabled, onZoom, usePageDate, saveOnZoom, xAxisIndex, -}: Omit): ZoomRenderProps { - const {handleChartReady} = useChartZoomCancel(); +}: UseChartZoomOptions): ZoomRenderProps { + const {handleChartReady} = useChartZoomCancel(disabled); const location = useLocation(); const navigate = useNavigate(); - /** - * Sets the new period due to a zoom related action - * - * Saves the current period to an instance property so that we - * can control URL state when zoom history is being manipulated - * by the chart controls. - * - * Saves a callback function to be called after chart animation is completed - */ - const setPeriod = useCallback( - (newPeriod: DateTimeUpdate) => { - const startFormatted = getQueryTime(newPeriod.start); - const endFormatted = getQueryTime(newPeriod.end); - - // Callback to let parent component know zoom has changed - // This is required for some more perceived responsiveness since - // we delay updating URL state so that chart animation can finish - // - // Parent container can use this to change into a loading state before - // URL parameters are changed - onZoom?.({ - period: newPeriod.period, - start: getQueryTime(newPeriod.start), - end: getQueryTime(newPeriod.end), - }); - + const commitZoomPeriod = useCallback( + (formattedPeriod: FormattedPeriod) => { if (usePageDate) { const newQuery = { ...location.query, - start: startFormatted, - end: endFormatted, - statsPeriod: newPeriod.period ?? undefined, + start: formattedPeriod.start, + end: formattedPeriod.end, + statsPeriod: formattedPeriod.period ?? undefined, }; // Only push new location if query params has changed because this will cause a heavy re-render @@ -174,95 +230,112 @@ export function useChartZoom({ }); } } else { - updateDateTime( - { - period: newPeriod.period, - start: startFormatted, - end: endFormatted, - }, - location, - navigate, - {save: saveOnZoom} - ); + updateDateTime(formattedPeriod, location, navigate, {save: saveOnZoom}); } }, - [onZoom, navigate, location, saveOnZoom, usePageDate] + [location, navigate, saveOnZoom, usePageDate] + ); + + const setPeriod = useCallback( + (newPeriod: DateTimeUpdate) => { + const formattedPeriod = getFormattedPeriod(newPeriod); + + // Callback to let parent component know zoom has changed. + onZoom?.(formattedPeriod); + + commitZoomPeriod(formattedPeriod); + }, + [commitZoomPeriod, onZoom] ); const handleDataZoom = useCallback( evt => { - let {startValue, endValue} = (evt as any).batch[0] as { - endValue: number | null; - startValue: number | null; - }; - - // if `rangeStart` and `rangeEnd` are null, then we are going back - if (startValue && endValue) { - // round off the bounds to the minute - startValue = Math.floor(startValue / 60_000) * 60_000; - endValue = Math.ceil(endValue / 60_000) * 60_000; - - // ensure the bounds has 1 minute resolution - startValue = Math.min(startValue, endValue - 60_000); - - setPeriod({ - period: null, - start: getUtcDateString(startValue), - end: getUtcDateString(endValue), - }); + if (disabled) { + return; } + + const range = getZoomRange(evt); + const roundedRange = range ? roundZoomRange(range) : null; + + // If the range values are null, ECharts is restoring zoom history. + if (!roundedRange) { + return; + } + + const {startValue, endValue} = roundedRange; + + setPeriod({ + period: null, + start: getUtcDateString(startValue), + end: getUtcDateString(endValue), + }); }, - [setPeriod] + [disabled, setPeriod] ); /** * Chart event when *any* rendering+animation finishes * - * `this.zooming` acts as a callback function so that - * we can let the native zoom animation on the chart complete - * before we update URL state and re-render + * Keep the hidden toolbox area-zoom cursor active after ECharts renders. */ - const handleChartFinished = useCallback((_props, chart) => { - activateZoomAreaSelect(chart); - }, []); + const handleChartFinished = useCallback( + (_props, chart) => { + if (disabled) { + return; + } + + activateZoomAreaSelect(chart); + }, + [disabled] + ); const dataZoomProp = useMemo(() => { + // Keep the inside dataZoom model even when disabled so synced charts can + // still receive x-range changes without this hook writing URL state. const zoomInside = DataZoomInside({ + id: 'useChartZoom-inside', xAxisIndex, }); return zoomInside; }, [xAxisIndex]); - const toolBox = useMemo( - () => - ToolBox( - {}, - { - dataZoom: { - title: { - zoom: '', - back: '', - }, - iconStyle: { - borderWidth: 0, - color: 'transparent', - opacity: 0, - }, + const toolBox = useMemo(() => { + if (disabled) { + // Remove the hidden toolbox while disabled so it cannot emit + // URL-changing brush selections. + return {}; + } + + return ToolBox( + {id: 'useChartZoom-toolbox'}, + { + dataZoom: { + xAxisIndex, + title: { + zoom: '', + back: '', }, - } - ), - [] - ); + iconStyle: { + borderWidth: 0, + color: 'transparent', + opacity: 0, + }, + }, + } + ); + }, [disabled, xAxisIndex]); - const renderProps: ZoomRenderProps = { - // Zooming only works when grouped by date - isGroupedByDate: true, - dataZoom: dataZoomProp, - toolBox, - onDataZoom: handleDataZoom, - onFinished: handleChartFinished, - onChartReady: handleChartReady, - }; + const renderProps = useMemo( + () => ({ + ...CHART_ZOOM_MERGE_OPTIONS, + dataZoom: dataZoomProp, + toolBox, + onDataZoom: handleDataZoom, + onFinished: handleChartFinished, + onChartReady: handleChartReady, + }), + [dataZoomProp, handleChartFinished, handleChartReady, handleDataZoom, toolBox] + ); return renderProps; } diff --git a/static/app/components/charts/utils.tsx b/static/app/components/charts/utils.tsx index ce3d51cd593b..5f9e26cedfd7 100644 --- a/static/app/components/charts/utils.tsx +++ b/static/app/components/charts/utils.tsx @@ -468,11 +468,17 @@ export function isChartHovered(chartRef: ReactEchartsRef | null) { * without first clicking the (hidden) toolbox icon. */ export function activateZoomAreaSelect(chart: ECharts) { - const toolboxView = (chart as any)._componentsViews?.find((view: any) => - view._features?.get?.('dataZoom') - ); - const dataZoomFeature = toolboxView?._features.get('dataZoom'); - if (dataZoomFeature && !dataZoomFeature._isZoomActive) { + // ECharts can expose more than one toolbox dataZoom feature after option + // updates. Only re-arm the hidden selector when none are active, otherwise + // duplicate brush controllers can stack during drag selection. + const dataZoomFeatures = ((chart as any)._componentsViews ?? []) + .map((view: any) => view._features?.get?.('dataZoom')) + .filter(Boolean); + + if ( + dataZoomFeatures.length > 0 && + dataZoomFeatures.every((feature: any) => !feature._isZoomActive) + ) { // Calling dispatchAction will re-trigger handleChartFinished chart.dispatchAction({ type: 'takeGlobalCursor', diff --git a/static/app/views/dashboards/widgets/timeSeriesWidget/timeSeriesWidgetVisualization.tsx b/static/app/views/dashboards/widgets/timeSeriesWidget/timeSeriesWidgetVisualization.tsx index 8a912ea8abb5..c84f2c78bef4 100644 --- a/static/app/views/dashboards/widgets/timeSeriesWidget/timeSeriesWidgetVisualization.tsx +++ b/static/app/views/dashboards/widgets/timeSeriesWidget/timeSeriesWidgetVisualization.tsx @@ -644,22 +644,6 @@ export function TimeSeriesWidgetVisualization(props: TimeSeriesWidgetVisualizati From cd836c7c9a16cb3a265e71bf9288ffce0757ccd8 Mon Sep 17 00:00:00 2001 From: Scott Cooper Date: Wed, 8 Jul 2026 10:28:39 -0700 Subject: [PATCH 2/3] test(charts): Fix useChartZoom rerender types renderHook inferred the initial props too narrowly, so typecheck failed when the test rerendered with disabled zoom. Pin the hook props type in the spec so it matches the actual hook API. Co-Authored-By: Codex --- static/app/components/charts/useChartZoom.spec.tsx | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/static/app/components/charts/useChartZoom.spec.tsx b/static/app/components/charts/useChartZoom.spec.tsx index a4bf0e39b17e..27a3bbdc5116 100644 --- a/static/app/components/charts/useChartZoom.spec.tsx +++ b/static/app/components/charts/useChartZoom.spec.tsx @@ -7,6 +7,8 @@ import {useChartZoom} from './useChartZoom'; const START_VALUE = Date.UTC(2026, 6, 3, 11, 1, 30); const END_VALUE = Date.UTC(2026, 6, 3, 16, 54, 30); +type UseChartZoomProps = Parameters[0]; + function dataZoomPayload( startValue = START_VALUE, endValue = END_VALUE @@ -21,7 +23,10 @@ function dataZoomPayload( describe('useChartZoom', () => { it('keeps zoom merge props and the inside dataZoom model across disabled rerenders', () => { - const {result, rerender} = renderHookWithProviders(useChartZoom, { + const {result, rerender} = renderHookWithProviders< + ReturnType, + UseChartZoomProps + >((props: UseChartZoomProps) => useChartZoom(props), { initialProps: {saveOnZoom: true}, }); @@ -59,7 +64,10 @@ describe('useChartZoom', () => { }); it('updates query params from a zoom event and no-ops while disabled', () => { - const {result, rerender, router} = renderHookWithProviders(useChartZoom, { + const {result, rerender, router} = renderHookWithProviders< + ReturnType, + UseChartZoomProps + >((props: UseChartZoomProps) => useChartZoom(props), { initialProps: { usePageDate: true, }, From d6544d7b7de49e06876df80377636a42d8f83b57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Josh=20Goldberg=20=E2=9C=A8?= Date: Thu, 9 Jul 2026 11:38:38 -0400 Subject: [PATCH 3/3] fix(ourlogs): stop drag-to-zoom writing multiple history entries (#119263) `LogsAutoRefreshContext` was missing the _"only `navigate()` if the query changed"_ logic from other places like `chartZoom`. I added it there, then added a shared `navigateIfQueryChanged` helper to deduplicate the half-dozen places it was used. Closes LOGS-383. Co-authored-by: Claude --- static/app/components/charts/chartZoom.tsx | 7 +- static/app/components/charts/useChartZoom.tsx | 10 +-- .../components/pageFilters/actions.spec.tsx | 17 +++-- static/app/components/pageFilters/actions.tsx | 15 ++-- .../app/utils/navigateIfQueryChanged.spec.tsx | 72 +++++++++++++++++++ static/app/utils/navigateIfQueryChanged.tsx | 21 ++++++ .../contexts/logs/logsAutoRefreshContext.tsx | 3 +- .../explore/logs/logsAutoRefresh.spec.tsx | 42 ++++++++++- .../spans/spansQueryParamsProvider.tsx | 15 +--- 9 files changed, 159 insertions(+), 43 deletions(-) create mode 100644 static/app/utils/navigateIfQueryChanged.spec.tsx create mode 100644 static/app/utils/navigateIfQueryChanged.tsx diff --git a/static/app/components/charts/chartZoom.tsx b/static/app/components/charts/chartZoom.tsx index ea723f342a1c..c24cfdf2dab9 100644 --- a/static/app/components/charts/chartZoom.tsx +++ b/static/app/components/charts/chartZoom.tsx @@ -7,7 +7,6 @@ import type { } from 'echarts'; import type {Location} from 'history'; import moment, {type MomentInput} from 'moment-timezone'; -import * as qs from 'query-string'; import {CHART_ZOOM_MERGE_OPTIONS} from 'sentry/components/charts/chartZoomConfig'; import {DataZoomInside} from 'sentry/components/charts/components/dataZoomInside'; @@ -22,6 +21,7 @@ import type { EChartRestoreHandler, } from 'sentry/types/echarts'; import {getUtcDateString, getUtcToLocalDateObject} from 'sentry/utils/dates'; +import {navigateIfQueryChanged} from 'sentry/utils/navigateIfQueryChanged'; import {useLocation} from 'sentry/utils/useLocation'; import type {ReactRouter3Navigate} from 'sentry/utils/useNavigate'; import {useNavigate} from 'sentry/utils/useNavigate'; @@ -170,10 +170,7 @@ class ChartZoom extends Component { pageStatsPeriod: period ?? undefined, }; - // Only push new location if query params has changed because this will cause a heavy re-render - if (qs.stringify(newQuery) !== qs.stringify(location.query)) { - navigate({pathname: location.pathname, query: newQuery}); - } + navigateIfQueryChanged(navigate, location, {query: newQuery}); } else { updateDateTime( { diff --git a/static/app/components/charts/useChartZoom.tsx b/static/app/components/charts/useChartZoom.tsx index 9a950011561b..8b0c3ef5132d 100644 --- a/static/app/components/charts/useChartZoom.tsx +++ b/static/app/components/charts/useChartZoom.tsx @@ -1,6 +1,5 @@ import {useCallback, useEffect, useMemo, useRef} from 'react'; import type {DataZoomComponentOption, ECharts, ToolboxComponentOption} from 'echarts'; -import * as qs from 'query-string'; import {CHART_ZOOM_MERGE_OPTIONS} from 'sentry/components/charts/chartZoomConfig'; import {DataZoomInside} from 'sentry/components/charts/components/dataZoomInside'; @@ -14,6 +13,7 @@ import type { EChartFinishedHandler, } from 'sentry/types/echarts'; import {getUtcDateString} from 'sentry/utils/dates'; +import {navigateIfQueryChanged} from 'sentry/utils/navigateIfQueryChanged'; import {useLocation} from 'sentry/utils/useLocation'; import {useNavigate} from 'sentry/utils/useNavigate'; @@ -222,13 +222,7 @@ export function useChartZoom({ statsPeriod: formattedPeriod.period ?? undefined, }; - // Only push new location if query params has changed because this will cause a heavy re-render - if (qs.stringify(newQuery) !== qs.stringify(location.query)) { - navigate({ - pathname: location.pathname, - query: newQuery, - }); - } + navigateIfQueryChanged(navigate, location, {query: newQuery}); } else { updateDateTime(formattedPeriod, location, navigate, {save: saveOnZoom}); } diff --git a/static/app/components/pageFilters/actions.spec.tsx b/static/app/components/pageFilters/actions.spec.tsx index fbb4b2812879..48ade3e2ecf4 100644 --- a/static/app/components/pageFilters/actions.spec.tsx +++ b/static/app/components/pageFilters/actions.spec.tsx @@ -648,7 +648,7 @@ describe('PageFilters ActionCreators', () => { expect(nav).toHaveBeenCalledWith( {pathname: '/test/', query: {project: ['1']}}, - {replace: false} + {replace: undefined} ); }); it('does not update history when queries are the same', () => { @@ -721,7 +721,7 @@ describe('PageFilters ActionCreators', () => { expect(nav).toHaveBeenCalledWith( {pathname: '/test/', query: {environment: ['new-env']}}, - {replace: false} + {replace: undefined} ); }); @@ -734,7 +734,7 @@ describe('PageFilters ActionCreators', () => { expect(nav).toHaveBeenCalledWith( {pathname: '/test/', query: {environment: ['new-env', 'another-env']}}, - {replace: false} + {replace: undefined} ); }); @@ -744,7 +744,10 @@ describe('PageFilters ActionCreators', () => { location: {pathname: '/test/', query: {environment: 'test'}}, }).location; updateEnvironments(null, location, nav); - expect(nav).toHaveBeenCalledWith({pathname: '/test/', query: {}}, {replace: false}); + expect(nav).toHaveBeenCalledWith( + {pathname: '/test/', query: {}}, + {replace: undefined} + ); }); it('does not override an absolute date selection', () => { @@ -785,7 +788,7 @@ describe('PageFilters ActionCreators', () => { expect(nav).toHaveBeenCalledWith( {pathname: '/test/', query: {statsPeriod: '24h'}}, - {replace: false} + {replace: undefined} ); }); @@ -798,7 +801,7 @@ describe('PageFilters ActionCreators', () => { expect(nav).toHaveBeenCalledWith( {pathname: '/test/', query: {statsPeriod: '24h'}}, - {replace: false} + {replace: undefined} ); }); @@ -818,7 +821,7 @@ describe('PageFilters ActionCreators', () => { pathname: '/test/', query: {start: '2020-03-22T00:53:38', end: '2020-04-21T00:53:38'}, }, - {replace: false} + {replace: undefined} ); }); }); diff --git a/static/app/components/pageFilters/actions.tsx b/static/app/components/pageFilters/actions.tsx index a390c0618053..23b969fc75a1 100644 --- a/static/app/components/pageFilters/actions.tsx +++ b/static/app/components/pageFilters/actions.tsx @@ -3,7 +3,6 @@ import type {Location} from 'history'; import isInteger from 'lodash/isInteger'; import omit from 'lodash/omit'; import pick from 'lodash/pick'; -import * as qs from 'query-string'; import { ALL_ACCESS_PROJECTS, @@ -28,6 +27,7 @@ import type {Environment, MinimalProject, Project} from 'sentry/types/project'; import {getUtcDateString} from 'sentry/utils/dates'; import {defined} from 'sentry/utils/defined'; import {isActiveSuperuser} from 'sentry/utils/isActiveSuperuser'; +import {navigateIfQueryChanged} from 'sentry/utils/navigateIfQueryChanged'; import type {ReactRouter3Navigate} from 'sentry/utils/useNavigate'; type EnvironmentId = Environment['id']; @@ -491,13 +491,12 @@ function updateParams( const newQuery = getNewQueryParams(obj, location.query, options); - // Only push new location if query params has changed because this will cause a heavy re-render - if (qs.stringify(newQuery) === qs.stringify(location.query)) { - return; - } - - const to = {pathname: location.pathname, query: newQuery}; - navigate(to, {replace: !!options?.replace}); + navigateIfQueryChanged( + navigate, + location, + {query: newQuery}, + {replace: options?.replace} + ); } /** diff --git a/static/app/utils/navigateIfQueryChanged.spec.tsx b/static/app/utils/navigateIfQueryChanged.spec.tsx new file mode 100644 index 000000000000..a29898c2db50 --- /dev/null +++ b/static/app/utils/navigateIfQueryChanged.spec.tsx @@ -0,0 +1,72 @@ +import {LocationFixture} from 'sentry-fixture/locationFixture'; + +import {navigateIfQueryChanged} from 'sentry/utils/navigateIfQueryChanged'; +import type {ReactRouter3Navigate} from 'sentry/utils/useNavigate'; + +describe('navigateIfQueryChanged', () => { + it('does not navigate when the query is unchanged', () => { + const navigate: ReactRouter3Navigate = jest.fn(); + const location = LocationFixture({query: {foo: 'bar'}}); + + navigateIfQueryChanged(navigate, location, {query: {foo: 'bar'}}); + + expect(navigate).not.toHaveBeenCalled(); + }); + + it('navigates when the query changed', () => { + const navigate: ReactRouter3Navigate = jest.fn(); + const location = LocationFixture({pathname: '/foo/', query: {foo: 'bar'}}); + + navigateIfQueryChanged(navigate, location, {query: {foo: 'baz'}}); + + expect(navigate).toHaveBeenCalledWith( + {pathname: '/foo/', query: {foo: 'baz'}}, + undefined + ); + }); + + it('does not navigate when just the query key order is changed', () => { + const navigate: ReactRouter3Navigate = jest.fn(); + const location = LocationFixture({query: {a: '1', b: '2'}}); + + navigateIfQueryChanged(navigate, location, {query: {b: '2', a: '1'}}); + + expect(navigate).not.toHaveBeenCalled(); + }); + + it('navigates with navigate options passed through when they are provided', () => { + const navigate: ReactRouter3Navigate = jest.fn(); + const location = LocationFixture({pathname: '/foo/', query: {foo: 'bar'}}); + + navigateIfQueryChanged(navigate, location, {query: {foo: 'baz'}}, {replace: true}); + + expect(navigate).toHaveBeenCalledWith( + {pathname: '/foo/', query: {foo: 'baz'}}, + {replace: true} + ); + }); + + it('navigates with defaulting the target pathname to the current location pathname when it is not provided', () => { + const navigate: ReactRouter3Navigate = jest.fn(); + const location = LocationFixture({pathname: '/current/', query: {foo: 'bar'}}); + + navigateIfQueryChanged(navigate, location, {query: {foo: 'baz'}}); + + expect(navigate).toHaveBeenCalledWith( + {pathname: '/current/', query: {foo: 'baz'}}, + undefined + ); + }); + + it('does not navigate when only the pathname changed', () => { + const navigate: ReactRouter3Navigate = jest.fn(); + const location = LocationFixture({pathname: '/foo/', query: {foo: 'bar'}}); + + navigateIfQueryChanged(navigate, location, { + pathname: '/other/', + query: {foo: 'bar'}, + }); + + expect(navigate).not.toHaveBeenCalled(); + }); +}); diff --git a/static/app/utils/navigateIfQueryChanged.tsx b/static/app/utils/navigateIfQueryChanged.tsx new file mode 100644 index 000000000000..0bce62279ba1 --- /dev/null +++ b/static/app/utils/navigateIfQueryChanged.tsx @@ -0,0 +1,21 @@ +import type {NavigateOptions} from 'react-router-dom'; +import type {Location} from 'history'; +import * as qs from 'query-string'; + +import type {ReactRouter3Navigate} from 'sentry/utils/useNavigate'; + +interface NavigateTarget { + query: Location['query']; + pathname?: string; +} + +export function navigateIfQueryChanged( + navigate: ReactRouter3Navigate, + location: Location, + target: NavigateTarget, + options?: NavigateOptions +): void { + if (qs.stringify(target.query) !== qs.stringify(location.query)) { + navigate({...target, pathname: target.pathname ?? location.pathname}, options); + } +} diff --git a/static/app/views/explore/contexts/logs/logsAutoRefreshContext.tsx b/static/app/views/explore/contexts/logs/logsAutoRefreshContext.tsx index d7f687c5369d..8610ff1c04d9 100644 --- a/static/app/views/explore/contexts/logs/logsAutoRefreshContext.tsx +++ b/static/app/views/explore/contexts/logs/logsAutoRefreshContext.tsx @@ -2,6 +2,7 @@ import {createContext, useCallback, useContext, useRef, useState} from 'react'; import {useQueryClient} from '@tanstack/react-query'; import type {Location} from 'history'; +import {navigateIfQueryChanged} from 'sentry/utils/navigateIfQueryChanged'; import {decodeInteger, decodeScalar} from 'sentry/utils/queryString'; import {useLocation} from 'sentry/utils/useLocation'; import {useNavigate} from 'sentry/utils/useNavigate'; @@ -166,7 +167,7 @@ export function useSetLogsAutoRefresh() { target.query[LOGS_AUTO_REFRESH_KEY] = autoRefresh; } - navigate(target); + navigateIfQueryChanged(navigate, location, target); }, [navigate, location, queryClient, queryKey, setPausedAt, currentPausedAt] ); diff --git a/static/app/views/explore/logs/logsAutoRefresh.spec.tsx b/static/app/views/explore/logs/logsAutoRefresh.spec.tsx index 2bc05f0c3d8c..87fd110beb68 100644 --- a/static/app/views/explore/logs/logsAutoRefresh.spec.tsx +++ b/static/app/views/explore/logs/logsAutoRefresh.spec.tsx @@ -5,13 +5,22 @@ import {act, render, screen, userEvent, waitFor} from 'sentry-test/reactTestingL import {PageFiltersStore} from 'sentry/components/pageFilters/store'; import {LogsAnalyticsPageSource} from 'sentry/utils/analytics/logsAnalyticsEvent'; -import {LOGS_AUTO_REFRESH_KEY} from 'sentry/views/explore/contexts/logs/logsAutoRefreshContext'; +import { + type AutoRefreshState, + LOGS_AUTO_REFRESH_KEY, + useSetLogsAutoRefresh, +} from 'sentry/views/explore/contexts/logs/logsAutoRefreshContext'; import {LogsPageDataProvider} from 'sentry/views/explore/contexts/logs/logsPageData'; import {LOGS_FIELDS_KEY} from 'sentry/views/explore/contexts/logs/logsPageParams'; import {LOGS_SORT_BYS_KEY} from 'sentry/views/explore/contexts/logs/sortBys'; import {AutorefreshToggle} from 'sentry/views/explore/logs/logsAutoRefresh'; import {LogsQueryParamsProvider} from 'sentry/views/explore/logs/logsQueryParamsProvider'; +function SetAutoRefreshButton({state}: {state: AutoRefreshState}) { + const setAutoRefresh = useSetLogsAutoRefresh(); + return ; +} + describe('LogsAutoRefresh Integration Tests', () => { const { organization, @@ -39,6 +48,7 @@ describe('LogsAutoRefresh Integration Tests', () => { beforeEach(() => { jest.clearAllMocks(); + jest.useRealTimers(); MockApiClient.clearMockResponses(); // Default API mock @@ -378,4 +388,34 @@ describe('LogsAutoRefresh Integration Tests', () => { // Eventually rate limited expect(router.location.query[LOGS_AUTO_REFRESH_KEY]).toBe('rate_limit'); }); + + it('does not navigate when auto-refresh is set to an unchanged state', async () => { + const {router} = renderWithProviders(, { + initialRouterConfig: routerConfig, + organization, + }); + + const keyBefore = router.location.key; + + await userEvent.click(screen.getByRole('button', {name: 'set-auto-refresh'})); + + expect(router.location.key).toBe(keyBefore); + expect(router.location.query[LOGS_AUTO_REFRESH_KEY]).toBeUndefined(); + }); + + it('navigates when the auto-refresh state actually changes', async () => { + const {router} = renderWithProviders(, { + initialRouterConfig: routerConfig, + organization, + }); + + const keyBefore = router.location.key; + + await userEvent.click(screen.getByRole('button', {name: 'set-auto-refresh'})); + + await waitFor(() => { + expect(router.location.query[LOGS_AUTO_REFRESH_KEY]).toBe('enabled'); + }); + expect(router.location.key).not.toBe(keyBefore); + }); }); diff --git a/static/app/views/explore/spans/spansQueryParamsProvider.tsx b/static/app/views/explore/spans/spansQueryParamsProvider.tsx index 1ec243d52128..bfaccb474a29 100644 --- a/static/app/views/explore/spans/spansQueryParamsProvider.tsx +++ b/static/app/views/explore/spans/spansQueryParamsProvider.tsx @@ -1,7 +1,7 @@ import type {ReactNode} from 'react'; import {useCallback, useMemo, useRef} from 'react'; -import type {Location} from 'history'; +import {navigateIfQueryChanged} from 'sentry/utils/navigateIfQueryChanged'; import {useLocation} from 'sentry/utils/useLocation'; import {useNavigate} from 'sentry/utils/useNavigate'; import {QueryParamsContextProvider} from 'sentry/views/explore/queryParams/context'; @@ -12,13 +12,6 @@ import { isDefaultFields, } from 'sentry/views/explore/spans/spansQueryParams'; -function isSameLocation(a: Location, b: Location): boolean { - if (a.pathname !== b.pathname) { - return false; - } - return JSON.stringify(a.query) === JSON.stringify(b.query); -} - interface SpansQueryParamsProviderProps { children: ReactNode; } @@ -45,11 +38,7 @@ export function SpansQueryParamsProvider({children}: SpansQueryParamsProviderPro writableQueryParams ); - // Only navigate if the target URL is different from current location - // This prevents duplicate history entries which can cause browser back button issues - if (!isSameLocation(locationRef.current, target)) { - navigate(target); - } + navigateIfQueryChanged(navigate, locationRef.current, target); }, [navigate] );