diff --git a/static/app/components/charts/chartZoom.tsx b/static/app/components/charts/chartZoom.tsx index bf7fb3b61ce9..c24cfdf2dab9 100644 --- a/static/app/components/charts/chartZoom.tsx +++ b/static/app/components/charts/chartZoom.tsx @@ -7,8 +7,8 @@ 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'; import {ToolBox} from 'sentry/components/charts/components/toolBox'; import {activateZoomAreaSelect} from 'sentry/components/charts/utils'; @@ -21,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'; @@ -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; @@ -167,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( { @@ -326,13 +326,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 +360,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..27a3bbdc5116 --- /dev/null +++ b/static/app/components/charts/useChartZoom.spec.tsx @@ -0,0 +1,117 @@ +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); + +type UseChartZoomProps = Parameters[0]; + +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< + ReturnType, + UseChartZoomProps + >((props: UseChartZoomProps) => useChartZoom(props), { + 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< + ReturnType, + UseChartZoomProps + >((props: UseChartZoomProps) => useChartZoom(props), { + 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..8b0c3ef5132d 100644 --- a/static/app/components/charts/useChartZoom.tsx +++ b/static/app/components/charts/useChartZoom.tsx @@ -1,7 +1,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'; @@ -13,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'; @@ -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,146 +202,134 @@ 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 - if (qs.stringify(newQuery) !== qs.stringify(location.query)) { - navigate({ - pathname: location.pathname, - query: newQuery, - }); - } + navigateIfQueryChanged(navigate, location, {query: newQuery}); } 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/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/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 setAutoRefresh(state)}>set-auto-refresh; +} + 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] ); diff --git a/static/app/views/issueDetails/eventGraph.tsx b/static/app/views/issueDetails/eventGraph.tsx index e279ae5af375..e3f85606ab61 100644 --- a/static/app/views/issueDetails/eventGraph.tsx +++ b/static/app/views/issueDetails/eventGraph.tsx @@ -231,6 +231,7 @@ export function EventGraph({ }, [groupStats]); const chartZoomProps = useChartZoom({ + disabled: disableZoomNavigation, saveOnZoom: true, }); @@ -566,12 +567,7 @@ export function EventGraph({ }, ...releaseBubbleXAxis, }} - {...(disableZoomNavigation - ? { - isGroupedByDate: true, - dataZoom: chartZoomProps.dataZoom, - } - : chartZoomProps)} + {...chartZoomProps} />