diff --git a/static/app/views/explore/metrics/hooks/useStableLabels.tsx b/static/app/views/explore/metrics/hooks/useStableLabels.tsx index 8f23396ecd75..31f50ff0fbba 100644 --- a/static/app/views/explore/metrics/hooks/useStableLabels.tsx +++ b/static/app/views/explore/metrics/hooks/useStableLabels.tsx @@ -77,11 +77,14 @@ function parseEquationIndex(label: string): number { */ 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,11 +92,20 @@ export function useStableLabels(queries: BaseMetricQuery[]) { getLabel(i: number): string { return labelsRef.current[i] ?? getFunctionLabel(i); }, + /** + * 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 ( @@ -106,15 +118,22 @@ 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; +} + +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 da21266889bb..90c2c5c552dd 100644 --- a/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx +++ b/static/app/views/explore/metrics/multiMetricsQueryParams.spec.tsx @@ -1,11 +1,22 @@ -import type {ReactNode} from 'react'; +import {useEffect, 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, + defaultMetricQuery, + encodeMetricQueryParams, +} from 'sentry/views/explore/metrics/metricQuery'; +import { + LocalMultiMetricsQueryParamsProvider, MultiMetricsQueryParamsProvider, useAddMetricQuery, useMultiMetricsQueryParams, @@ -18,6 +29,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(); @@ -39,6 +66,40 @@ 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} + + ); +} + +function LocalWrapper({children}: {children: ReactNode}) { + return ( + + decodeMetricsQueryParams(canonicalMetricQueryParam(name))! + )} + > + {children} + + ); +} + describe('MultiMetricsQueryParamsProvider', () => { it('sets defaults', () => { const {result} = renderHookWithProviders(useMultiMetricsQueryParams, { @@ -469,6 +530,290 @@ 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); + }); + + 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', + ]); + }); + + 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 + ); + }); + + 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', + ]) + ); + }); + + 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', + ]); + }); + + 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(); + }); + + 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, { @@ -612,7 +957,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 +982,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 +1006,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 +1041,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 +1073,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 +1115,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..9efa3f3b45ae 100644 --- a/static/app/views/explore/metrics/multiMetricsQueryParams.tsx +++ b/static/app/views/explore/metrics/multiMetricsQueryParams.tsx @@ -3,16 +3,16 @@ import { useCallback, useContext, useMemo, + useRef, 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 +51,45 @@ 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]); + + // 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( - (nextQueries: BaseMetricQuery[]) => { - const target = {...location, query: {...location.query}}; - target.query.metric = encodeMetricQueries(nextQueries); - navigateIfQueryChanged(navigate, location, target); + (update: SetStateAction) => { + let nextRawQueries: string[] | undefined; + const flushed = setRawQueries(({metric: stateRawQueries}) => { + const previousRawQueries = pendingRawQueries.current ?? stateRawQueries; + const nextQueries = + typeof update === 'function' + ? update(decodeMetricQueries(previousRawQueries)) + : update; + nextRawQueries = encodeMetricQueries(nextQueries); + if (isEqual(nextRawQueries, previousRawQueries)) { + return {}; + } + pendingRawQueries.current = nextRawQueries; + return {metric: nextRawQueries}; + }); + void flushed.finally(() => { + if (pendingRawQueries.current === nextRawQueries) { + pendingRawQueries.current = null; + } + }); }, - [location, navigate] + [setRawQueries] ); const value = useMetricQueriesController({queries, setQueries}); @@ -80,16 +101,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 c484cafd8e09..444b69d1d34c 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 { @@ -20,7 +20,6 @@ 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, @@ -39,6 +38,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'}) => string; metricQueries: MetricQuery[]; @@ -49,9 +89,14 @@ export interface MetricQueriesControllerValue { ) => void; } +type RowMutators = Pick< + MetricQuery, + 'setQueryParams' | 'setTraceMetric' | 'removeMetric' +>; + interface UseMetricQueriesControllerArgs { queries: BaseMetricQuery[]; - setQueries: (nextQueries: BaseMetricQuery[]) => void; + setQueries: Dispatch>; } /** @@ -68,97 +113,69 @@ export function useMetricQueriesController({ }: UseMetricQueriesControllerArgs): MetricQueriesControllerValue { const labels = useStableLabels(queries); - 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, - }; - } + const getRowMutators = useMemo(() => { + function updateQuery( + rowId: string, + update: (metricQuery: BaseMetricQuery) => BaseMetricQuery + ) { + // 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: rowLabels[j], + })); + const newMetricQueries = labeledQueries.map((metricQuery, j) => + j === index ? update(metricQuery) : metricQuery ); - setQueries(syncUpdatedMetricQueries(labeledQueries, newMetricQueries)); - }; + return 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, - }; + function createRowMutators(rowId: string): RowMutators { + return { + setQueryParams: newQueryParams => + updateQuery(rowId, metricQuery => ({ + metric: metricQuery.metric, + queryParams: newQueryParams, + label: metricQuery.label, + })), + setTraceMetric: newTraceMetric => + updateQuery(rowId, metricQuery => withTraceMetric(metricQuery, newTraceMetric)), + removeMetric: () => { + const rowIds = labels.getRowIds(); + const index = rowIds.indexOf(rowId); + if (index === -1 || rowIds.length <= 1) { + return; } - ); - setQueries(syncUpdatedMetricQueries(labeledQueries, newMetricQueries)); + // 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)); + }, }; } - function removeMetricQueryForIndex(i: number) { - return function () { - if (labeledQueries.length <= 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)); - }; - } + return createRowMutators; + }, [setQueries, labels]); + + const rowIdsKey = labels.getRowIds().join(','); + const rowMutators = useMemo( + () => rowIdsKey.split(',').map(getRowMutators), + [rowIdsKey, getRowMutators] + ); + + return useMemo(() => { + const labeledQueries: BaseMetricQuery[] = queries.map((query, i) => ({ + ...query, + label: labels.getLabel(i), + })); function addMetricQuery({ type = 'aggregate', @@ -195,14 +212,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} + + ); }