From f512f71a209179041359cf28d56e941c8d0753cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Josh=20Goldberg=20=E2=9C=A8?= Date: Wed, 21 Oct 2026 11:34:17 -0400 Subject: [PATCH 1/6] perf(metrics): Keep unchanged metric panels stable when another panel changes Row mutators hand updater functions to setQueries instead of closing over the rendered queries, and decoded queries are cached per URL value and index, so unchanged panels keep the same query params and mutators. The URL-backed provider moves onto nuqs, whose setter is stable and composes queued writes. --- .../metrics/multiMetricsQueryParams.spec.tsx | 98 +++++++++- .../metrics/multiMetricsQueryParams.tsx | 65 ++++--- .../metrics/useMetricQueriesController.tsx | 176 +++++++++--------- tests/js/sentry-test/nuqsTestingAdapter.tsx | 14 +- 4 files changed, 227 insertions(+), 126 deletions(-) diff --git a/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx b/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx index da21266889bb..cb5a64321048 100644 --- a/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx +++ b/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx @@ -1,10 +1,19 @@ import type {ReactNode} from 'react'; import {OrganizationFixture} from 'sentry-fixture/organization'; -import {act, renderHookWithProviders, screen} from 'sentry-test/reactTestingLibrary'; +import { + act, + renderHookWithProviders, + screen, + waitFor, +} from 'sentry-test/reactTestingLibrary'; import {EQUATION_PREFIX} from 'sentry/utils/discover/fields'; import {Mode} from 'sentry/views/explore/contexts/pageParamsContext/mode'; +import { + decodeMetricsQueryParams, + encodeMetricQueryParams, +} from 'sentry/views/explore/metrics/metricQuery'; import { MultiMetricsQueryParamsProvider, useAddMetricQuery, @@ -18,6 +27,22 @@ import { } from 'sentry/views/explore/queryParams/visualize'; import {ChartType} from 'sentry/views/insights/common/components/chart'; +function canonicalMetricQueryParam(name: string) { + return encodeMetricQueryParams( + decodeMetricsQueryParams( + JSON.stringify({ + metric: {name, type: 'counter'}, + query: '', + aggregateFields: [ + new VisualizeFunction(`sum(value,${name},counter,none)`).serialize(), + ], + aggregateSortBys: [], + mode: 'samples', + }) + )! + ); +} + function TestableMetricComponent() { const metricQueries = useMultiMetricsQueryParams(); @@ -469,6 +494,65 @@ describe('MultiMetricsQueryParamsProvider', () => { ]); }); + it('keeps other queries referentially stable when one query changes', () => { + const {result} = renderHookWithProviders(useMultiMetricsQueryParams, { + additionalWrapper: Wrapper, + initialRouterConfig: { + location: { + pathname: '/organizations/org-slug/explore/metrics/', + query: { + metric: ['foo', 'bar'].map(canonicalMetricQueryParam), + }, + }, + }, + }); + const unchanged = result.current[1]!; + + act(() => result.current[0]!.setTraceMetric({name: 'baz', type: 'counter'})); + + expect(result.current[0]!.metric).toEqual({name: 'baz', type: 'counter'}); + expect(result.current[1]!.metric).toBe(unchanged.metric); + expect(result.current[1]!.queryParams).toBe(unchanged.queryParams); + expect(result.current[1]!.setQueryParams).toBe(unchanged.setQueryParams); + expect(result.current[1]!.setTraceMetric).toBe(unchanged.setTraceMetric); + expect(result.current[1]!.removeMetric).toBe(unchanged.removeMetric); + }); + + it('keeps both updates when two queries change in the same tick', () => { + const {result} = renderHookWithProviders(useMultiMetricsQueryParams, { + additionalWrapper: Wrapper, + initialRouterConfig: { + location: { + pathname: '/organizations/org-slug/explore/metrics/', + query: {metric: ['foo', 'bar'].map(canonicalMetricQueryParam)}, + }, + }, + }); + const [first, second] = result.current; + + act(() => { + first!.setTraceMetric({name: 'baz', type: 'counter'}); + second!.setQueryParams(second!.queryParams.replace({query: 'span.op:db'})); + }); + + expect(result.current[0]!.metric).toEqual({name: 'baz', type: 'counter'}); + expect(result.current[1]!.queryParams.query).toBe('span.op:db'); + }); + + it('does not share decoded queries between panels with identical URL values', () => { + const {result} = renderHookWithProviders(useMultiMetricsQueryParams, { + additionalWrapper: Wrapper, + initialRouterConfig: { + location: { + pathname: '/organizations/org-slug/explore/metrics/', + query: {metric: ['foo', 'foo'].map(canonicalMetricQueryParam)}, + }, + }, + }); + + expect(result.current[1]!.queryParams).not.toBe(result.current[0]!.queryParams); + }); + describe('stable labels', () => { it('preserves label B when A is deleted from [A, B]', () => { const {result} = renderHookWithProviders(useMultiMetricsQueryParams, { @@ -612,7 +696,7 @@ describe('MultiMetricsQueryParamsProvider', () => { }); describe('useAddMetricQuery', () => { - it('adds new metric at the end of the list when adding without equations', () => { + it('adds new metric at the end of the list when adding without equations', async () => { const {result, router} = renderHookWithProviders(useAddMetricQuery, { additionalWrapper: Wrapper, initialRouterConfig: { @@ -637,7 +721,7 @@ describe('MultiMetricsQueryParamsProvider', () => { act(() => result.current()); - expect(router.location.query.metric).toHaveLength(2); + await waitFor(() => expect(router.location.query.metric).toHaveLength(2)); expect(JSON.parse(router.location.query.metric![0]!)).toEqual( expect.objectContaining({ @@ -661,7 +745,7 @@ describe('MultiMetricsQueryParamsProvider', () => { ); }); - it('duplicates the last metric when adding with equations', () => { + it('duplicates the last metric when adding with equations', async () => { const {result, router} = renderHookWithProviders(useAddMetricQuery, { additionalWrapper: Wrapper, organization: OrganizationFixture({ @@ -696,7 +780,7 @@ describe('MultiMetricsQueryParamsProvider', () => { act(() => result.current()); - expect(router.location.query.metric).toHaveLength(3); + await waitFor(() => expect(router.location.query.metric).toHaveLength(3)); expect(JSON.parse(router.location.query.metric![0]!)).toEqual( expect.objectContaining({ @@ -728,7 +812,7 @@ describe('MultiMetricsQueryParamsProvider', () => { ); }); - it('adds equations to the end of the list', () => { + it('adds equations to the end of the list', async () => { const {result, router} = renderHookWithProviders(useAddMetricQuery, { additionalWrapper: Wrapper, organization: OrganizationFixture({ @@ -770,7 +854,7 @@ describe('MultiMetricsQueryParamsProvider', () => { act(() => result.current()); - expect(router.location.query.metric).toHaveLength(3); + await waitFor(() => expect(router.location.query.metric).toHaveLength(3)); expect(JSON.parse(router.location.query.metric![0]!)).toEqual( expect.objectContaining({ diff --git a/static/app/views/explore/metrics/multiMetricsQueryParams.tsx b/static/app/views/explore/metrics/multiMetricsQueryParams.tsx index 64dd25731df8..8d6d025631fb 100644 --- a/static/app/views/explore/metrics/multiMetricsQueryParams.tsx +++ b/static/app/views/explore/metrics/multiMetricsQueryParams.tsx @@ -5,14 +5,13 @@ import { useMemo, useState, type ReactNode, + type SetStateAction, } from 'react'; -import type {Location} from 'history'; +import isEqual from 'lodash/isEqual'; +import {useQueryStates} from 'nuqs'; import {defined} from 'sentry/utils/defined'; -import {navigateIfQueryChanged} from 'sentry/utils/navigateIfQueryChanged'; -import {decodeList} from 'sentry/utils/queryString'; -import {useLocation} from 'sentry/utils/useLocation'; -import {useNavigate} from 'sentry/utils/useNavigate'; +import {parseAsStringArray} from 'sentry/utils/url/parseAsStringArray'; import { decodeMetricsQueryParams, defaultMetricQuery, @@ -51,24 +50,31 @@ interface MultiMetricsQueryParamsProviderProps { children: ReactNode; } +const metricQueryParsers = { + metric: parseAsStringArray.withOptions({history: 'push'}), +}; + export function MultiMetricsQueryParamsProvider({ children, }: MultiMetricsQueryParamsProviderProps) { - const location = useLocation(); - const navigate = useNavigate(); + const [{metric: rawQueries}, setRawQueries] = useQueryStates(metricQueryParsers); - const queries = useMemo( - () => getMultiMetricsQueryParamsFromLocation(location), - [location] - ); + const queries = useMemo(() => decodeMetricQueries(rawQueries), [rawQueries]); const setQueries = useCallback( - (nextQueries: BaseMetricQuery[]) => { - const target = {...location, query: {...location.query}}; - target.query.metric = encodeMetricQueries(nextQueries); - navigateIfQueryChanged(navigate, location, target); + (update: SetStateAction) => { + void setRawQueries(({metric: previousRawQueries}) => { + const nextQueries = + typeof update === 'function' + ? update(decodeMetricQueries(previousRawQueries)) + : update; + const nextRawQueries = encodeMetricQueries(nextQueries); + return isEqual(nextRawQueries, previousRawQueries) + ? {} + : {metric: nextRawQueries}; + }); }, - [location, navigate] + [setRawQueries] ); const value = useMetricQueriesController({queries, setQueries}); @@ -80,16 +86,27 @@ export function MultiMetricsQueryParamsProvider({ ); } -function getMultiMetricsQueryParamsFromLocation(location: Location): BaseMetricQuery[] { - const rawQueryParams = decodeList(location.query.metric); - - const metricQueries = rawQueryParams - .map(value => decodeMetricsQueryParams(value)) - .filter(defined); +const decodedMetricQueries = new Map(); +const MAX_DECODED_METRIC_QUERIES = 100; + +function decodeMetricQueryParamsCached( + value: string, + index: number +): BaseMetricQuery | null { + const key = `${index}:${value}`; + if (!decodedMetricQueries.has(key)) { + if (decodedMetricQueries.size >= MAX_DECODED_METRIC_QUERIES) { + decodedMetricQueries.clear(); + } + decodedMetricQueries.set(key, decodeMetricsQueryParams(value)); + } + return decodedMetricQueries.get(key) ?? null; +} - const queries = metricQueries.length ? metricQueries : [defaultMetricQuery()]; +function decodeMetricQueries(rawQueries: string[]): BaseMetricQuery[] { + const metricQueries = rawQueries.map(decodeMetricQueryParamsCached).filter(defined); - return queries; + return metricQueries.length ? metricQueries : [defaultMetricQuery()]; } interface LocalMultiMetricsQueryParamsProviderProps { diff --git a/static/app/views/explore/metrics/useMetricQueriesController.tsx b/static/app/views/explore/metrics/useMetricQueriesController.tsx index 3b090776ac4e..62dbda834649 100644 --- a/static/app/views/explore/metrics/useMetricQueriesController.tsx +++ b/static/app/views/explore/metrics/useMetricQueriesController.tsx @@ -1,4 +1,4 @@ -import {useMemo} from 'react'; +import {useMemo, type Dispatch, type SetStateAction} from 'react'; import {determineDefaultChartType} from 'sentry/views/explore/contexts/pageParamsContext/visualizes'; import { @@ -39,6 +39,47 @@ function syncUpdatedMetricQueries( ); } +function withTraceMetric( + metricQuery: BaseMetricQuery, + newTraceMetric: TraceMetric +): BaseMetricQuery { + // When changing trace metrics, adjust the currently selected + // aggregation so it's valid for the new metric's type. + const visualize = metricQuery.queryParams.visualizes[0]; + let aggregateFields: Array | undefined; + if (visualize && isVisualizeFunction(visualize)) { + const selectedAggregation = visualize.parsedFunction?.name; + const allowedAggregations = OPTIONS_BY_TYPE[newTraceMetric.type]; + const aggregation = + selectedAggregation && + allowedAggregations?.find(option => option.value === selectedAggregation) + ? selectedAggregation + : DEFAULT_YAXIS_BY_TYPE[newTraceMetric.type] || 'sum'; + + let updatedVisualize = updateVisualizeYAxis(visualize, aggregation, newTraceMetric); + + if ( + visualize.chartType === ChartType.HEATMAP && + !doesMetricSupportHeatMapVisualization(newTraceMetric) + ) { + updatedVisualize = updatedVisualize.replace({ + chartType: determineDefaultChartType([updatedVisualize.yAxis]), + }); + } + + aggregateFields = [ + updatedVisualize, + ...metricQuery.queryParams.aggregateFields.filter(isGroupBy), + ]; + } + + return { + queryParams: metricQuery.queryParams.replace({aggregateFields}), + metric: newTraceMetric, + label: metricQuery.label, + }; +} + export interface MetricQueriesControllerValue { addMetricQuery: (options?: {type?: 'aggregate' | 'equation'}) => void; metricQueries: MetricQuery[]; @@ -51,7 +92,7 @@ export interface MetricQueriesControllerValue { interface UseMetricQueriesControllerArgs { queries: BaseMetricQuery[]; - setQueries: (nextQueries: BaseMetricQuery[]) => void; + setQueries: Dispatch>; } /** @@ -67,98 +108,53 @@ export function useMetricQueriesController({ setQueries, }: UseMetricQueriesControllerArgs): MetricQueriesControllerValue { const labels = useStableLabels(queries); + const queryCount = queries.length; - return useMemo(() => { - const labeledQueries: BaseMetricQuery[] = queries.map((query, i) => ({ - ...query, - label: labels.getLabel(i), - setQueryParams: () => {}, - setTraceMetric: () => {}, - removeMetric: () => {}, - })); - - function setQueryParamsForIndex(i: number) { - return function (newQueryParams: ReadableQueryParams) { - const newMetricQueries = labeledQueries.map( - (metricQuery: BaseMetricQuery, j: number) => { - if (i !== j) { - return metricQuery; - } - return { - metric: metricQuery.metric, - queryParams: newQueryParams, - label: metricQuery.label, - }; - } - ); - setQueries(syncUpdatedMetricQueries(labeledQueries, newMetricQueries)); - }; - } - - function setTraceMetricForIndex(i: number) { - return function (newTraceMetric: TraceMetric) { - const newMetricQueries = labeledQueries.map( - (metricQuery: BaseMetricQuery, j: number) => { - if (i !== j) { - return metricQuery; - } - - // When changing trace metrics, adjust the currently selected - // aggregation so it's valid for the new metric's type. - const visualize = metricQuery.queryParams.visualizes[0]; - let aggregateFields: Array | undefined; - if (visualize && isVisualizeFunction(visualize)) { - const selectedAggregation = visualize.parsedFunction?.name; - const allowedAggregations = OPTIONS_BY_TYPE[newTraceMetric.type]; - const aggregation = - selectedAggregation && - allowedAggregations?.find(option => option.value === selectedAggregation) - ? selectedAggregation - : DEFAULT_YAXIS_BY_TYPE[newTraceMetric.type] || 'sum'; - - let updatedVisualize = updateVisualizeYAxis( - visualize, - aggregation, - newTraceMetric - ); - - if ( - visualize.chartType === ChartType.HEATMAP && - !doesMetricSupportHeatMapVisualization(newTraceMetric) - ) { - updatedVisualize = updatedVisualize.replace({ - chartType: determineDefaultChartType([updatedVisualize.yAxis]), - }); - } - - aggregateFields = [ - updatedVisualize, - ...metricQuery.queryParams.aggregateFields.filter(isGroupBy), - ]; - } - - return { - queryParams: metricQuery.queryParams.replace({aggregateFields}), - metric: newTraceMetric, - label: metricQuery.label, - }; - } + const rowMutators = useMemo(() => { + function updateQueryAtIndex( + i: number, + update: (metricQuery: BaseMetricQuery) => BaseMetricQuery + ) { + setQueries(previousQueries => { + const labeledQueries = previousQueries.map((metricQuery, j) => ({ + ...metricQuery, + label: labels.getLabel(j), + })); + const newMetricQueries = labeledQueries.map((metricQuery, j) => + i === j ? update(metricQuery) : metricQuery ); - setQueries(syncUpdatedMetricQueries(labeledQueries, newMetricQueries)); - }; + return syncUpdatedMetricQueries(labeledQueries, newMetricQueries); + }); } - function removeMetricQueryForIndex(i: number) { - return function () { - if (labeledQueries.length <= 1) { + return Array.from({length: queryCount}, (_, i) => ({ + setQueryParams: (newQueryParams: ReadableQueryParams) => + updateQueryAtIndex(i, metricQuery => ({ + metric: metricQuery.metric, + queryParams: newQueryParams, + label: metricQuery.label, + })), + setTraceMetric: (newTraceMetric: TraceMetric) => + updateQueryAtIndex(i, metricQuery => + withTraceMetric(metricQuery, newTraceMetric) + ), + removeMetric: () => { + if (queryCount <= 1) { return; } // Update the label ref synchronously before the storage write so // labels stay stable on the next render. labels.remove(i); - setQueries(labeledQueries.filter((_, j) => i !== j)); - }; - } + setQueries(previousQueries => previousQueries.toSpliced(i, 1)); + }, + })); + }, [queryCount, setQueries, labels]); + + return useMemo(() => { + const labeledQueries: BaseMetricQuery[] = queries.map((query, i) => ({ + ...query, + label: labels.getLabel(i), + })); function addMetricQuery({ type = 'aggregate', @@ -194,14 +190,12 @@ export function useMetricQueriesController({ } return { - metricQueries: labeledQueries.map((metric, index) => ({ - ...metric, - setQueryParams: setQueryParamsForIndex(index), - setTraceMetric: setTraceMetricForIndex(index), - removeMetric: removeMetricQueryForIndex(index), + metricQueries: labeledQueries.map((metricQuery, i) => ({ + ...metricQuery, + ...rowMutators[i]!, })), addMetricQuery, reorderMetricQueries, }; - }, [queries, setQueries, labels]); + }, [queries, setQueries, labels, rowMutators]); } diff --git a/tests/js/sentry-test/nuqsTestingAdapter.tsx b/tests/js/sentry-test/nuqsTestingAdapter.tsx index ee0031ee1318..376456a9a092 100644 --- a/tests/js/sentry-test/nuqsTestingAdapter.tsx +++ b/tests/js/sentry-test/nuqsTestingAdapter.tsx @@ -5,7 +5,7 @@ import { renderQueryString, } from 'nuqs/adapters/custom'; import type {unstable_AdapterInterface as AdapterInterface} from 'nuqs/adapters/custom'; -import type {OnUrlUpdateFunction} from 'nuqs/adapters/testing'; +import {NuqsTestingAdapter, type OnUrlUpdateFunction} from 'nuqs/adapters/testing'; import {useLocation} from 'sentry/utils/useLocation'; import {useNavigate} from 'sentry/utils/useNavigate'; @@ -84,8 +84,14 @@ export function SentryNuqsTestingAdapter({ onUrlUpdate, }: SentryNuqsTestingAdapterProps): ReactElement { return ( - - {children} - + // nuqs keeps its URL update queue in a global, and only its own testing + // adapter clears it on mount, so a previous test's pending write would + // otherwise become this test's initial state. The inner provider overrides + // its adapter. + + + {children} + + ); } From e468c99f37b3ff2d95b657002cce69d5314d6ce6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Josh=20Goldberg=20=E2=9C=A8?= Date: Wed, 21 Oct 2026 12:45:06 -0400 Subject: [PATCH 2/6] fix(metrics): Resolve metric row mutators by label at call time Index-bound mutators edited the wrong row when captured before an earlier row was removed. Adds coverage for unrelated URL changes, no-op writes, and history pushes. --- .../explore/metrics/hooks/useStableLabels.tsx | 3 + .../metrics/multiMetricsQueryParams.spec.tsx | 103 ++++++++++++++++++ .../metrics/useMetricQueriesController.tsx | 80 +++++++++----- 3 files changed, 156 insertions(+), 30 deletions(-) diff --git a/static/app/views/explore/metrics/hooks/useStableLabels.tsx b/static/app/views/explore/metrics/hooks/useStableLabels.tsx index 8f23396ecd75..24414455be9c 100644 --- a/static/app/views/explore/metrics/hooks/useStableLabels.tsx +++ b/static/app/views/explore/metrics/hooks/useStableLabels.tsx @@ -89,6 +89,9 @@ export function useStableLabels(queries: BaseMetricQuery[]) { getLabel(i: number): string { return labelsRef.current[i] ?? getFunctionLabel(i); }, + getLabels(): readonly string[] { + return labelsRef.current; + }, insert(position: number, label: string) { labelsRef.current = labelsRef.current.toSpliced(position, 0, label); }, diff --git a/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx b/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx index cb5a64321048..59167f3fe6e7 100644 --- a/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx +++ b/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx @@ -553,6 +553,109 @@ describe('MultiMetricsQueryParamsProvider', () => { expect(result.current[1]!.queryParams).not.toBe(result.current[0]!.queryParams); }); + it('keeps every query referentially stable when an unrelated URL param changes', async () => { + const metric = ['foo', 'bar'].map(canonicalMetricQueryParam); + const {result, router} = renderHookWithProviders(useMultiMetricsQueryParams, { + additionalWrapper: Wrapper, + initialRouterConfig: { + location: {pathname: '/organizations/org-slug/explore/metrics/', query: {metric}}, + }, + }); + const [first, second] = result.current; + const search = new URLSearchParams(metric.map(value => ['metric', value])); + search.set('unrelated', '1'); + + router.navigate(`/organizations/org-slug/explore/metrics/?${search.toString()}`); + await waitFor(() => expect(router.location.query.unrelated).toBe('1')); + + expect(result.current[0]).toBe(first); + expect(result.current[1]).toBe(second); + }); + + it('applies a captured mutator to its own query after an earlier query is removed', () => { + const {result} = renderHookWithProviders(useMultiMetricsQueryParams, { + additionalWrapper: Wrapper, + initialRouterConfig: { + location: { + pathname: '/organizations/org-slug/explore/metrics/', + query: {metric: ['foo', 'bar', 'baz'].map(canonicalMetricQueryParam)}, + }, + }, + }); + const third = result.current[2]!; + + act(() => result.current[0]!.removeMetric()); + act(() => third.setQueryParams(third.queryParams.replace({query: 'span.op:db'}))); + + expect(result.current.map(metricQuery => metricQuery.metric.name)).toEqual([ + 'bar', + 'baz', + ]); + expect(result.current.map(metricQuery => metricQuery.queryParams.query)).toEqual([ + '', + 'span.op:db', + ]); + }); + + describe('URL writes', () => { + beforeEach(() => { + jest.useFakeTimers(); + }); + + afterEach(async () => { + try { + await act(() => jest.runOnlyPendingTimersAsync()); + } finally { + jest.useRealTimers(); + } + }); + + it('does not write the URL when an update leaves the queries unchanged', () => { + const onNuqsUrlUpdate = jest.fn(); + const {result} = renderHookWithProviders(useMultiMetricsQueryParams, { + additionalWrapper: Wrapper, + onNuqsUrlUpdate, + initialRouterConfig: { + location: { + pathname: '/organizations/org-slug/explore/metrics/', + query: {metric: ['foo'].map(canonicalMetricQueryParam)}, + }, + }, + }); + + act(() => result.current[0]!.setQueryParams(result.current[0]!.queryParams)); + act(() => jest.runAllTimers()); + + expect(onNuqsUrlUpdate).not.toHaveBeenCalled(); + }); + + it('pushes a history entry when an update changes the queries', () => { + const onNuqsUrlUpdate = jest.fn(); + const {result} = renderHookWithProviders(useMultiMetricsQueryParams, { + additionalWrapper: Wrapper, + onNuqsUrlUpdate, + initialRouterConfig: { + location: { + pathname: '/organizations/org-slug/explore/metrics/', + query: {metric: ['foo'].map(canonicalMetricQueryParam)}, + }, + }, + }); + + act(() => + result.current[0]!.setQueryParams( + result.current[0]!.queryParams.replace({query: 'span.op:db'}) + ) + ); + act(() => jest.runAllTimers()); + + expect(onNuqsUrlUpdate).toHaveBeenCalledTimes(1); + expect(onNuqsUrlUpdate).toHaveBeenCalledWith( + expect.objectContaining({options: expect.objectContaining({history: 'push'})}) + ); + }); + }); + describe('stable labels', () => { it('preserves label B when A is deleted from [A, B]', () => { const {result} = renderHookWithProviders(useMultiMetricsQueryParams, { diff --git a/static/app/views/explore/metrics/useMetricQueriesController.tsx b/static/app/views/explore/metrics/useMetricQueriesController.tsx index 6912d38d0715..4ab86d463b4b 100644 --- a/static/app/views/explore/metrics/useMetricQueriesController.tsx +++ b/static/app/views/explore/metrics/useMetricQueriesController.tsx @@ -20,12 +20,12 @@ import { } from 'sentry/views/explore/metrics/metricQuery'; import {updateVisualizeYAxis} from 'sentry/views/explore/metrics/utils'; import {isGroupBy, type GroupBy} from 'sentry/views/explore/queryParams/groupBy'; -import type {ReadableQueryParams} from 'sentry/views/explore/queryParams/readableQueryParams'; import { isVisualizeEquation, isVisualizeFunction, VisualizeFunction, } from 'sentry/views/explore/queryParams/visualize'; +import {getFunctionLabel} from 'sentry/views/explore/utils/getFunctionLabel'; import {ChartType} from 'sentry/views/insights/common/components/chart'; function syncUpdatedMetricQueries( @@ -90,6 +90,11 @@ export interface MetricQueriesControllerValue { ) => void; } +type RowMutators = Pick< + MetricQuery, + 'setQueryParams' | 'setTraceMetric' | 'removeMetric' +>; + interface UseMetricQueriesControllerArgs { queries: BaseMetricQuery[]; setQueries: Dispatch>; @@ -108,47 +113,62 @@ export function useMetricQueriesController({ setQueries, }: UseMetricQueriesControllerArgs): MetricQueriesControllerValue { const labels = useStableLabels(queries); - const queryCount = queries.length; - const rowMutators = useMemo(() => { - function updateQueryAtIndex( - i: number, + const getRowMutators = useMemo(() => { + function updateQuery( + label: string, update: (metricQuery: BaseMetricQuery) => BaseMetricQuery ) { + // Snapshot labels at call time: a deferred useState updater sees only the + // calls made before this one, and so does this snapshot. + const currentLabels = labels.getLabels(); setQueries(previousQueries => { const labeledQueries = previousQueries.map((metricQuery, j) => ({ ...metricQuery, - label: labels.getLabel(j), + label: currentLabels[j] ?? getFunctionLabel(j), })); - const newMetricQueries = labeledQueries.map((metricQuery, j) => - i === j ? update(metricQuery) : metricQuery + if (!labeledQueries.some(metricQuery => metricQuery.label === label)) { + return previousQueries; + } + const newMetricQueries = labeledQueries.map(metricQuery => + metricQuery.label === label ? update(metricQuery) : metricQuery ); return syncUpdatedMetricQueries(labeledQueries, newMetricQueries); }); } - return Array.from({length: queryCount}, (_, i) => ({ - setQueryParams: (newQueryParams: ReadableQueryParams) => - updateQueryAtIndex(i, metricQuery => ({ - metric: metricQuery.metric, - queryParams: newQueryParams, - label: metricQuery.label, - })), - setTraceMetric: (newTraceMetric: TraceMetric) => - updateQueryAtIndex(i, metricQuery => - withTraceMetric(metricQuery, newTraceMetric) - ), - removeMetric: () => { - if (queryCount <= 1) { - return; - } - // Update the label ref synchronously before the storage write so - // labels stay stable on the next render. - labels.remove(i); - setQueries(previousQueries => previousQueries.toSpliced(i, 1)); - }, - })); - }, [queryCount, setQueries, labels]); + function createRowMutators(label: string): RowMutators { + return { + setQueryParams: newQueryParams => + updateQuery(label, metricQuery => ({ + metric: metricQuery.metric, + queryParams: newQueryParams, + label: metricQuery.label, + })), + setTraceMetric: newTraceMetric => + updateQuery(label, metricQuery => withTraceMetric(metricQuery, newTraceMetric)), + removeMetric: () => { + const currentLabels = labels.getLabels(); + const index = currentLabels.indexOf(label); + if (index === -1 || currentLabels.length <= 1) { + return; + } + // Update the label ref synchronously before the storage write so + // labels stay stable on the next render. + labels.remove(index); + setQueries(previousQueries => previousQueries.toSpliced(index, 1)); + }, + }; + } + + return createRowMutators; + }, [setQueries, labels]); + + const queryLabelsKey = queries.map((_, i) => labels.getLabel(i)).join(','); + const rowMutators = useMemo( + () => queryLabelsKey.split(',').map(getRowMutators), + [queryLabelsKey, getRowMutators] + ); return useMemo(() => { const labeledQueries: BaseMetricQuery[] = queries.map((query, i) => ({ From 67f5506abedd8dfd125c8d27e79d6baef670f354 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Josh=20Goldberg=20=E2=9C=A8?= Date: Wed, 21 Oct 2026 13:00:08 -0400 Subject: [PATCH 3/6] fix(metrics): Resolve metric row mutators by a unique row id Labels can repeat after an external navigation that keeps the row count, so mutators resolved by label could edit or remove the wrong row. --- .../explore/metrics/hooks/useStableLabels.tsx | 34 ++++++++---- .../metrics/multiMetricsQueryParams.spec.tsx | 52 +++++++++++++++++++ .../metrics/useMetricQueriesController.tsx | 41 ++++++++------- 3 files changed, 98 insertions(+), 29 deletions(-) diff --git a/static/app/views/explore/metrics/hooks/useStableLabels.tsx b/static/app/views/explore/metrics/hooks/useStableLabels.tsx index 24414455be9c..cc6e88966332 100644 --- a/static/app/views/explore/metrics/hooks/useStableLabels.tsx +++ b/static/app/views/explore/metrics/hooks/useStableLabels.tsx @@ -75,13 +75,22 @@ function parseEquationIndex(label: string): number { * When the ref length doesn't match the query count (fresh load or external * navigation), labels are reassigned sequentially. */ +let nextRowId = 0; + +function createRowIds(count: number): string[] { + return Array.from({length: count}, () => String(nextRowId++)); +} + export function useStableLabels(queries: BaseMetricQuery[]) { const labelsRef = useRef([]); + const rowIdsRef = useRef([]); // oxlint-disable-next-line react/refs if (labelsRef.current.length !== queries.length) { // oxlint-disable-next-line react/refs labelsRef.current = assignSequentialLabels(queries); + // oxlint-disable-next-line react/refs + rowIdsRef.current = createRowIds(queries.length); } return useMemo( @@ -89,14 +98,20 @@ export function useStableLabels(queries: BaseMetricQuery[]) { getLabel(i: number): string { return labelsRef.current[i] ?? getFunctionLabel(i); }, - getLabels(): readonly string[] { - return labelsRef.current; + /** + * Unique per row and moved along with its label, unlike labels, which + * can repeat after an external navigation that keeps the row count. + */ + getRowIds(): readonly string[] { + return rowIdsRef.current; }, insert(position: number, label: string) { labelsRef.current = labelsRef.current.toSpliced(position, 0, label); + rowIdsRef.current = rowIdsRef.current.toSpliced(position, 0, ...createRowIds(1)); }, remove(position: number) { labelsRef.current = labelsRef.current.filter((_, j) => j !== position); + rowIdsRef.current = rowIdsRef.current.filter((_, j) => j !== position); }, move(from: number, to: number) { if ( @@ -109,15 +124,16 @@ export function useStableLabels(queries: BaseMetricQuery[]) { return; } - const next = [...labelsRef.current]; - const [label] = next.splice(from, 1); - if (!label) { - return; - } - next.splice(to, 0, label); - labelsRef.current = next; + labelsRef.current = moveItem(labelsRef.current, from, to); + rowIdsRef.current = moveItem(rowIdsRef.current, from, to); }, }), [] ); } + +function moveItem(items: readonly T[], from: number, to: number): T[] { + const next = [...items]; + next.splice(to, 0, ...next.splice(from, 1)); + return next; +} diff --git a/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx b/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx index 59167f3fe6e7..0dd7160614eb 100644 --- a/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx +++ b/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx @@ -597,6 +597,58 @@ describe('MultiMetricsQueryParamsProvider', () => { ]); }); + it('updates only its own query when two queries share a label', async () => { + const aggregate = canonicalMetricQueryParam('foo'); + const equation = JSON.stringify({ + metric: {name: '', type: ''}, + query: '', + aggregateFields: [new VisualizeEquation('equation|A * 2').serialize()], + aggregateSortBys: [], + mode: 'aggregate', + }); + const {result, router} = renderHookWithProviders( + () => ({ + addMetricQuery: useAddMetricQuery(), + metricQueries: useMultiMetricsQueryParams(), + }), + { + additionalWrapper: Wrapper, + initialRouterConfig: { + location: { + pathname: '/organizations/org-slug/explore/metrics/', + query: {metric: [aggregate, canonicalMetricQueryParam('bar')]}, + }, + }, + } + ); + const search = new URLSearchParams([ + ['metric', aggregate], + ['metric', equation], + ]); + router.navigate(`/organizations/org-slug/explore/metrics/?${search.toString()}`); + await waitFor(() => + expect(result.current.metricQueries[1]!.queryParams.visualizes[0]).toBeInstanceOf( + VisualizeEquation + ) + ); + act(() => result.current.addMetricQuery()); + const added = result.current.metricQueries[1]!; + + act(() => added.setQueryParams(added.queryParams.replace({query: 'span.op:db'}))); + + expect(result.current.metricQueries.map(metricQuery => metricQuery.label)).toEqual([ + 'A', + 'B', + 'B', + ]); + expect( + result.current.metricQueries.map(metricQuery => metricQuery.queryParams.query) + ).toEqual(['', 'span.op:db', '']); + expect(result.current.metricQueries[2]!.queryParams.visualizes[0]).toBeInstanceOf( + VisualizeEquation + ); + }); + describe('URL writes', () => { beforeEach(() => { jest.useFakeTimers(); diff --git a/static/app/views/explore/metrics/useMetricQueriesController.tsx b/static/app/views/explore/metrics/useMetricQueriesController.tsx index 4ab86d463b4b..444b69d1d34c 100644 --- a/static/app/views/explore/metrics/useMetricQueriesController.tsx +++ b/static/app/views/explore/metrics/useMetricQueriesController.tsx @@ -25,7 +25,6 @@ import { isVisualizeFunction, VisualizeFunction, } from 'sentry/views/explore/queryParams/visualize'; -import {getFunctionLabel} from 'sentry/views/explore/utils/getFunctionLabel'; import {ChartType} from 'sentry/views/insights/common/components/chart'; function syncUpdatedMetricQueries( @@ -116,41 +115,43 @@ export function useMetricQueriesController({ const getRowMutators = useMemo(() => { function updateQuery( - label: string, + rowId: string, update: (metricQuery: BaseMetricQuery) => BaseMetricQuery ) { - // Snapshot labels at call time: a deferred useState updater sees only the - // calls made before this one, and so does this snapshot. - const currentLabels = labels.getLabels(); + // Read rows at call time: a deferred useState updater sees only the calls + // made before this one, and so does this snapshot. + const rowIds = labels.getRowIds(); + const rowLabels = rowIds.map((_, j) => labels.getLabel(j)); + const index = rowIds.indexOf(rowId); setQueries(previousQueries => { + if (index === -1 || previousQueries.length !== rowIds.length) { + return previousQueries; + } const labeledQueries = previousQueries.map((metricQuery, j) => ({ ...metricQuery, - label: currentLabels[j] ?? getFunctionLabel(j), + label: rowLabels[j], })); - if (!labeledQueries.some(metricQuery => metricQuery.label === label)) { - return previousQueries; - } - const newMetricQueries = labeledQueries.map(metricQuery => - metricQuery.label === label ? update(metricQuery) : metricQuery + const newMetricQueries = labeledQueries.map((metricQuery, j) => + j === index ? update(metricQuery) : metricQuery ); return syncUpdatedMetricQueries(labeledQueries, newMetricQueries); }); } - function createRowMutators(label: string): RowMutators { + function createRowMutators(rowId: string): RowMutators { return { setQueryParams: newQueryParams => - updateQuery(label, metricQuery => ({ + updateQuery(rowId, metricQuery => ({ metric: metricQuery.metric, queryParams: newQueryParams, label: metricQuery.label, })), setTraceMetric: newTraceMetric => - updateQuery(label, metricQuery => withTraceMetric(metricQuery, newTraceMetric)), + updateQuery(rowId, metricQuery => withTraceMetric(metricQuery, newTraceMetric)), removeMetric: () => { - const currentLabels = labels.getLabels(); - const index = currentLabels.indexOf(label); - if (index === -1 || currentLabels.length <= 1) { + const rowIds = labels.getRowIds(); + const index = rowIds.indexOf(rowId); + if (index === -1 || rowIds.length <= 1) { return; } // Update the label ref synchronously before the storage write so @@ -164,10 +165,10 @@ export function useMetricQueriesController({ return createRowMutators; }, [setQueries, labels]); - const queryLabelsKey = queries.map((_, i) => labels.getLabel(i)).join(','); + const rowIdsKey = labels.getRowIds().join(','); const rowMutators = useMemo( - () => queryLabelsKey.split(',').map(getRowMutators), - [queryLabelsKey, getRowMutators] + () => rowIdsKey.split(',').map(getRowMutators), + [rowIdsKey, getRowMutators] ); return useMemo(() => { From c28bc583ea771abc45a8680b3e086d70a7ef7ba6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Josh=20Goldberg=20=E2=9C=A8?= Date: Wed, 21 Oct 2026 13:14:01 -0400 Subject: [PATCH 4/6] fix(metrics): Keep metric query writes made while panels mount nuqs only tracks its own writes after the provider's effects subscribe, so a second write during the mount commit started from the stale URL and dropped the first. --- .../metrics/multiMetricsQueryParams.spec.tsx | 45 ++++++++++++++++++- .../metrics/multiMetricsQueryParams.tsx | 25 ++++++++--- 2 files changed, 64 insertions(+), 6 deletions(-) diff --git a/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx b/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx index 0dd7160614eb..8d105428e53f 100644 --- a/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx +++ b/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx @@ -1,4 +1,4 @@ -import type {ReactNode} from 'react'; +import {useEffect, type ReactNode} from 'react'; import {OrganizationFixture} from 'sentry-fixture/organization'; import { @@ -12,6 +12,7 @@ import {EQUATION_PREFIX} from 'sentry/utils/discover/fields'; import {Mode} from 'sentry/views/explore/contexts/pageParamsContext/mode'; import { decodeMetricsQueryParams, + defaultMetricQuery, encodeMetricQueryParams, } from 'sentry/views/explore/metrics/metricQuery'; import { @@ -64,6 +65,28 @@ function Wrapper({children}: {children: ReactNode}) { ); } +function SelectMetricWhenEmpty({index}: {index: number}) { + const {metric, setTraceMetric} = useMultiMetricsQueryParams()[index]!; + + useEffect(() => { + if (!metric.name) { + setTraceMetric({name: `metric${index}`, type: 'counter'}); + } + }, [index, metric.name, setTraceMetric]); + + return null; +} + +function SelectMetricsWhenEmptyWrapper({children}: {children: ReactNode}) { + return ( + + + + {children} + + ); +} + describe('MultiMetricsQueryParamsProvider', () => { it('sets defaults', () => { const {result} = renderHookWithProviders(useMultiMetricsQueryParams, { @@ -649,6 +672,26 @@ describe('MultiMetricsQueryParamsProvider', () => { ); }); + it('keeps every update made by panels as they mount', async () => { + const emptyMetricQuery = encodeMetricQueryParams(defaultMetricQuery()); + const {result} = renderHookWithProviders(useMultiMetricsQueryParams, { + additionalWrapper: SelectMetricsWhenEmptyWrapper, + initialRouterConfig: { + location: { + pathname: '/organizations/org-slug/explore/metrics/', + query: {metric: [emptyMetricQuery, emptyMetricQuery]}, + }, + }, + }); + + await waitFor(() => + expect(result.current.map(metricQuery => metricQuery.metric.name)).toEqual([ + 'metric0', + 'metric1', + ]) + ); + }); + describe('URL writes', () => { beforeEach(() => { jest.useFakeTimers(); diff --git a/static/app/views/explore/metrics/multiMetricsQueryParams.tsx b/static/app/views/explore/metrics/multiMetricsQueryParams.tsx index 8d6d025631fb..9efa3f3b45ae 100644 --- a/static/app/views/explore/metrics/multiMetricsQueryParams.tsx +++ b/static/app/views/explore/metrics/multiMetricsQueryParams.tsx @@ -3,6 +3,7 @@ import { useCallback, useContext, useMemo, + useRef, useState, type ReactNode, type SetStateAction, @@ -61,17 +62,31 @@ export function MultiMetricsQueryParamsProvider({ const queries = useMemo(() => decodeMetricQueries(rawQueries), [rawQueries]); + // nuqs only sees its own writes once this provider's effects subscribe, which + // is after children's mount effects. Until a write flushes, build on it here + // so a second write in the same mount doesn't start from the stale URL. + const pendingRawQueries = useRef(null); + const setQueries = useCallback( (update: SetStateAction) => { - void setRawQueries(({metric: previousRawQueries}) => { + let nextRawQueries: string[] | undefined; + const flushed = setRawQueries(({metric: stateRawQueries}) => { + const previousRawQueries = pendingRawQueries.current ?? stateRawQueries; const nextQueries = typeof update === 'function' ? update(decodeMetricQueries(previousRawQueries)) : update; - const nextRawQueries = encodeMetricQueries(nextQueries); - return isEqual(nextRawQueries, previousRawQueries) - ? {} - : {metric: nextRawQueries}; + nextRawQueries = encodeMetricQueries(nextQueries); + if (isEqual(nextRawQueries, previousRawQueries)) { + return {}; + } + pendingRawQueries.current = nextRawQueries; + return {metric: nextRawQueries}; + }); + void flushed.finally(() => { + if (pendingRawQueries.current === nextRawQueries) { + pendingRawQueries.current = null; + } }); }, [setRawQueries] From 04bc0bf49b2a720fa2af0a233fe05d3578300536 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Josh=20Goldberg=20=E2=9C=A8?= Date: Wed, 21 Oct 2026 13:29:27 -0400 Subject: [PATCH 5/6] test(metrics): Cover external navigation after a pending metric query write --- .../metrics/multiMetricsQueryParams.spec.tsx | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx b/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx index 8d105428e53f..13a58de4570d 100644 --- a/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx +++ b/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx @@ -692,6 +692,34 @@ describe('MultiMetricsQueryParamsProvider', () => { ); }); + it('builds on an external navigation after its own write lands', async () => { + const {result, router} = renderHookWithProviders(useMultiMetricsQueryParams, { + additionalWrapper: Wrapper, + initialRouterConfig: { + location: { + pathname: '/organizations/org-slug/explore/metrics/', + query: {metric: ['foo', 'bar'].map(canonicalMetricQueryParam)}, + }, + }, + }); + + act(() => result.current[0]!.setTraceMetric({name: 'written', type: 'counter'})); + await waitFor(() => + expect(router.location.query.metric![0]).toContain('"name":"written"') + ); + const search = new URLSearchParams( + ['external', 'other'].map(name => ['metric', canonicalMetricQueryParam(name)]) + ); + router.navigate(`/organizations/org-slug/explore/metrics/?${search.toString()}`); + await waitFor(() => expect(result.current[0]!.metric.name).toBe('external')); + act(() => result.current[1]!.setTraceMetric({name: 'edited', type: 'counter'})); + + expect(result.current.map(metricQuery => metricQuery.metric.name)).toEqual([ + 'external', + 'edited', + ]); + }); + describe('URL writes', () => { beforeEach(() => { jest.useFakeTimers(); From 184ec8900eac4d89104ca4ce4e4de59000f31fd1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Josh=20Goldberg=20=E2=9C=A8?= Date: Wed, 21 Oct 2026 13:49:07 -0400 Subject: [PATCH 6/6] test(metrics): Cover deferred local metric query updates Also keeps the useStableLabels docblock attached to the hook. --- .../explore/metrics/hooks/useStableLabels.tsx | 12 +++---- .../metrics/multiMetricsQueryParams.spec.tsx | 35 +++++++++++++++++++ 2 files changed, 41 insertions(+), 6 deletions(-) diff --git a/static/app/views/explore/metrics/hooks/useStableLabels.tsx b/static/app/views/explore/metrics/hooks/useStableLabels.tsx index cc6e88966332..31f50ff0fbba 100644 --- a/static/app/views/explore/metrics/hooks/useStableLabels.tsx +++ b/static/app/views/explore/metrics/hooks/useStableLabels.tsx @@ -75,12 +75,6 @@ function parseEquationIndex(label: string): number { * When the ref length doesn't match the query count (fresh load or external * navigation), labels are reassigned sequentially. */ -let nextRowId = 0; - -function createRowIds(count: number): string[] { - return Array.from({length: count}, () => String(nextRowId++)); -} - export function useStableLabels(queries: BaseMetricQuery[]) { const labelsRef = useRef([]); const rowIdsRef = useRef([]); @@ -137,3 +131,9 @@ function moveItem(items: readonly T[], from: number, to: number): T[] { next.splice(to, 0, ...next.splice(from, 1)); return next; } + +let nextRowId = 0; + +function createRowIds(count: number): string[] { + return Array.from({length: count}, () => String(nextRowId++)); +} diff --git a/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx b/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx index 13a58de4570d..90c2c5c552dd 100644 --- a/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx +++ b/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx @@ -16,6 +16,7 @@ import { encodeMetricQueryParams, } from 'sentry/views/explore/metrics/metricQuery'; import { + LocalMultiMetricsQueryParamsProvider, MultiMetricsQueryParamsProvider, useAddMetricQuery, useMultiMetricsQueryParams, @@ -87,6 +88,18 @@ function SelectMetricsWhenEmptyWrapper({children}: {children: ReactNode}) { ); } +function LocalWrapper({children}: {children: ReactNode}) { + return ( + + decodeMetricsQueryParams(canonicalMetricQueryParam(name))! + )} + > + {children} + + ); +} + describe('MultiMetricsQueryParamsProvider', () => { it('sets defaults', () => { const {result} = renderHookWithProviders(useMultiMetricsQueryParams, { @@ -720,6 +733,28 @@ describe('MultiMetricsQueryParamsProvider', () => { ]); }); + it('applies same-tick updates in order when local state defers them', () => { + const {result} = renderHookWithProviders(useMultiMetricsQueryParams, { + additionalWrapper: LocalWrapper, + }); + const [first, , third] = result.current; + + act(() => { + first!.setQueryParams(first!.queryParams.replace({query: 'span.op:http'})); + third!.setQueryParams(third!.queryParams.replace({query: 'span.op:db'})); + first!.removeMetric(); + }); + + expect(result.current.map(metricQuery => metricQuery.metric.name)).toEqual([ + 'bar', + 'baz', + ]); + expect(result.current.map(metricQuery => metricQuery.queryParams.query)).toEqual([ + '', + 'span.op:db', + ]); + }); + describe('URL writes', () => { beforeEach(() => { jest.useFakeTimers();