From 953b430259b8ec26215228b19193b3f1747abb9b Mon Sep 17 00:00:00 2001 From: nikkikapadia Date: Tue, 10 Dec 2024 16:42:18 -0500 Subject: [PATCH 1/7] add limit field to widget builder hook --- .../hooks/useWidgetBuilderState.spec.tsx | 25 ++++++++++++++ .../hooks/useWidgetBuilderState.tsx | 34 ++++++++++++++++--- .../widgetBuilder/widgetBuilder.tsx | 1 + 3 files changed, 56 insertions(+), 4 deletions(-) diff --git a/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.spec.tsx b/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.spec.tsx index 20ef2fe084e354..73c1c9646df23c 100644 --- a/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.spec.tsx +++ b/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.spec.tsx @@ -243,4 +243,29 @@ describe('useWidgetBuilderState', () => { expect(result.current.state.sort).toEqual([{field: 'testField', kind: 'asc'}]); }); }); + + describe('limit', () => { + it('can decode and update limit', () => { + mockedUsedLocation.mockReturnValue( + LocationFixture({ + query: { + limit: '4', + }, + }) + ); + + const {result} = renderHook(() => useWidgetBuilderState()); + + expect(result.current.state.limit).toEqual(4); + + act(() => { + result.current.dispatch({ + type: BuilderStateAction.SET_LIMIT, + payload: 10, + }); + }); + + expect(result.current.state.limit).toEqual(10); + }); + }); }); diff --git a/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.tsx b/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.tsx index a0aa5412c524a3..4926dd86dd61a0 100644 --- a/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.tsx +++ b/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.tsx @@ -6,9 +6,15 @@ import { generateFieldAsString, type Sort, } from 'sentry/utils/discover/fields'; -import {decodeList, decodeSorts} from 'sentry/utils/queryString'; +import { + decodeInteger, + decodeList, + decodeScalar, + decodeSorts, +} from 'sentry/utils/queryString'; import {DisplayType, WidgetType} from 'sentry/views/dashboards/types'; import {useQueryParamState} from 'sentry/views/dashboards/widgetBuilder/hooks/useQueryParamState'; +import {DEFAULT_RESULTS_LIMIT} from 'sentry/views/dashboards/widgetBuilder/utils'; export type WidgetBuilderStateQueryParams = { dataset?: WidgetType; @@ -30,6 +36,7 @@ export const BuilderStateAction = { SET_Y_AXIS: 'SET_Y_AXIS', SET_QUERY: 'SET_QUERY', SET_SORT: 'SET_SORT', + SET_LIMIT: 'SET_LIMIT', } as const; type WidgetAction = @@ -40,13 +47,15 @@ type WidgetAction = | {payload: Column[]; type: typeof BuilderStateAction.SET_FIELDS} | {payload: Column[]; type: typeof BuilderStateAction.SET_Y_AXIS} | {payload: string[]; type: typeof BuilderStateAction.SET_QUERY} - | {payload: Sort[]; type: typeof BuilderStateAction.SET_SORT}; + | {payload: Sort[]; type: typeof BuilderStateAction.SET_SORT} + | {payload: number; type: typeof BuilderStateAction.SET_LIMIT}; export interface WidgetBuilderState { dataset?: WidgetType; description?: string; displayType?: DisplayType; fields?: Column[]; + limit?: number; query?: string[]; sort?: Sort[]; title?: string; @@ -90,10 +99,15 @@ function useWidgetBuilderState(): { decoder: decodeSorts, serializer: serializeSorts, }); + const [limit, setLimit] = useQueryParamState({ + fieldName: 'limit', + decoder: decodeScalar, + deserializer: deserializeLimit, + }); const state = useMemo( - () => ({title, description, displayType, dataset, fields, yAxis, query, sort}), - [title, description, displayType, dataset, fields, yAxis, query, sort] + () => ({title, description, displayType, dataset, fields, yAxis, query, sort, limit}), + [title, description, displayType, dataset, fields, yAxis, query, sort, limit] ); const dispatch = useCallback( @@ -126,6 +140,9 @@ function useWidgetBuilderState(): { case BuilderStateAction.SET_SORT: setSort(action.payload); break; + case BuilderStateAction.SET_LIMIT: + setLimit(action.payload); + break; default: break; } @@ -139,6 +156,7 @@ function useWidgetBuilderState(): { setYAxis, setQuery, setSort, + setLimit, ] ); @@ -193,4 +211,12 @@ function serializeSorts(sorts: Sort[]): string[] { }); } +/** + * Decodes the limit from the query params + * Returns the default limit if the value is not a valid limit + */ +function deserializeLimit(value: string): number { + return decodeInteger(value, DEFAULT_RESULTS_LIMIT); +} + export default useWidgetBuilderState; diff --git a/static/app/views/dashboards/widgetBuilder/widgetBuilder.tsx b/static/app/views/dashboards/widgetBuilder/widgetBuilder.tsx index f9a20b234a3ea0..ce297ab1821824 100644 --- a/static/app/views/dashboards/widgetBuilder/widgetBuilder.tsx +++ b/static/app/views/dashboards/widgetBuilder/widgetBuilder.tsx @@ -760,6 +760,7 @@ function WidgetBuilder({ // The grouping was cleared, so clear the orderby newQuery.orderby = ''; } else if (!newQuery.orderby) { + // THIS ONE ############################# const orderOptions = generateOrderOptions({ widgetType: widgetType ?? defaultWidgetType, columns: query.columns, From 3e3a72d03fd935781fce50a9977e0fc3473e7aec Mon Sep 17 00:00:00 2001 From: nikkikapadia Date: Tue, 10 Dec 2024 16:57:20 -0500 Subject: [PATCH 2/7] ffs i left in a comment --- static/app/views/dashboards/widgetBuilder/widgetBuilder.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/static/app/views/dashboards/widgetBuilder/widgetBuilder.tsx b/static/app/views/dashboards/widgetBuilder/widgetBuilder.tsx index ce297ab1821824..f9a20b234a3ea0 100644 --- a/static/app/views/dashboards/widgetBuilder/widgetBuilder.tsx +++ b/static/app/views/dashboards/widgetBuilder/widgetBuilder.tsx @@ -760,7 +760,6 @@ function WidgetBuilder({ // The grouping was cleared, so clear the orderby newQuery.orderby = ''; } else if (!newQuery.orderby) { - // THIS ONE ############################# const orderOptions = generateOrderOptions({ widgetType: widgetType ?? defaultWidgetType, columns: query.columns, From 84d293d22f5a98b2f6bd36966689e686acec0508 Mon Sep 17 00:00:00 2001 From: nikkikapadia Date: Wed, 11 Dec 2024 11:43:10 -0500 Subject: [PATCH 3/7] integrate limit into sort by selector --- .../components/sortBySelector.tsx | 28 +++++++++++++------ 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/static/app/views/dashboards/widgetBuilder/components/sortBySelector.tsx b/static/app/views/dashboards/widgetBuilder/components/sortBySelector.tsx index fb519389871f1b..bb8fe24c3f4a49 100644 --- a/static/app/views/dashboards/widgetBuilder/components/sortBySelector.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/sortBySelector.tsx @@ -1,4 +1,4 @@ -import {Fragment, useState} from 'react'; +import {Fragment, useEffect} from 'react'; import styled from '@emotion/styled'; import SelectControl from 'sentry/components/forms/controls/selectControl'; @@ -16,7 +16,6 @@ import {SectionHeader} from 'sentry/views/dashboards/widgetBuilder/components/co import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {BuilderStateAction} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; import { - DEFAULT_RESULTS_LIMIT, getResultsLimit, SortDirection, } from 'sentry/views/dashboards/widgetBuilder/utils'; @@ -27,8 +26,6 @@ function WidgetBuilderSortBySelector() { const {state, dispatch} = useWidgetBuilderContext(); const widget = convertBuilderStateToWidget(state); - const [limit, setLimit] = useState(DEFAULT_RESULTS_LIMIT); - const datasetConfig = getDatasetConfig(state.dataset); let tags: TagCollection = useTags(); @@ -60,6 +57,19 @@ function WidgetBuilderSortBySelector() { widget.queries[0].aggregates.length ); + // handles when the maxLimit changes to a value less than the current selected limit + useEffect(() => { + if (!state.limit) { + return; + } + if (state.limit > maxLimit) { + dispatch({ + type: BuilderStateAction.SET_LIMIT, + payload: maxLimit, + }); + } + }, [state.limit, maxLimit, dispatch]); + function handleSortByChange(newSortBy: string, sortDirection: 'asc' | 'desc') { dispatch({ type: BuilderStateAction.SET_SORT, @@ -83,7 +93,7 @@ function WidgetBuilderSortBySelector() { flexibleControlStateSize stacked > - {isTimeseriesChart && limit && ( + {isTimeseriesChart && state.limit && ( ) => { - // TODO: implement this when limit is implemented in widget builder state - setLimit(option.value); + dispatch({ + type: BuilderStateAction.SET_LIMIT, + payload: option.value, + }); }} /> )} From 2197725620958d6d5ea8b91e6a6af5091448cf06 Mon Sep 17 00:00:00 2001 From: nikkikapadia Date: Wed, 11 Dec 2024 13:31:31 -0500 Subject: [PATCH 4/7] add tests for limit changes --- .../components/sortBySelector.spec.tsx | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/static/app/views/dashboards/widgetBuilder/components/sortBySelector.spec.tsx b/static/app/views/dashboards/widgetBuilder/components/sortBySelector.spec.tsx index 31c51c0661c454..4268e1ad70573e 100644 --- a/static/app/views/dashboards/widgetBuilder/components/sortBySelector.spec.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/sortBySelector.spec.tsx @@ -22,6 +22,7 @@ const {organization, router} = initializeOrg({ query: { displayType: 'line', fields: ['transaction.duration', 'count()', 'id'], + yAxis: ['count()', 'count_unique(transaction.duration)'], }, }, params: {}, @@ -123,4 +124,66 @@ describe('WidgetBuilderSortBySelector', function () { }) ); }); + + it('renders the correct limit options', async function () { + // const mockNavigate = jest.fn(); + // mockUseNavigate.mockReturnValue(mockNavigate); + + render( + + + + + , + {router, organization} + ); + + // default limit is 5 + expect(await screen.findByText('Limit to 5 results')).toBeInTheDocument(); + + const moreAggregatesRouter = RouterFixture({ + ...router, + location: { + ...router.location, + query: { + ...router.location.query, + yAxis: ['count()', 'count_unique(transaction.duration)', 'eps()'], + }, + }, + }); + + render( + + + + + , + {router: moreAggregatesRouter, organization} + ); + + // default limit changes to 3 since its the max limit for 3 aggregates + expect(await screen.findByText('Limit to 3 results')).toBeInTheDocument(); + }); + + it('correctly handles limit changes', async function () { + const mockNavigate = jest.fn(); + mockUseNavigate.mockReturnValue(mockNavigate); + + render( + + + + + , + {router, organization} + ); + + const limitSelector = await screen.findByText('Limit to 5 results'); + await userEvent.click(limitSelector); + await userEvent.click(await screen.findByText('Limit to 3 results')); + + expect(mockNavigate).toHaveBeenLastCalledWith( + expect.objectContaining({query: expect.objectContaining({limit: 3})}) + ); + }); }); From 5ab0ce7006df5abc8abd8ef9c2feb2528ffe97f3 Mon Sep 17 00:00:00 2001 From: nikkikapadia Date: Wed, 11 Dec 2024 13:37:13 -0500 Subject: [PATCH 5/7] forgot comments --- .../widgetBuilder/components/sortBySelector.spec.tsx | 3 --- 1 file changed, 3 deletions(-) diff --git a/static/app/views/dashboards/widgetBuilder/components/sortBySelector.spec.tsx b/static/app/views/dashboards/widgetBuilder/components/sortBySelector.spec.tsx index 4268e1ad70573e..6200d2aba258eb 100644 --- a/static/app/views/dashboards/widgetBuilder/components/sortBySelector.spec.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/sortBySelector.spec.tsx @@ -126,9 +126,6 @@ describe('WidgetBuilderSortBySelector', function () { }); it('renders the correct limit options', async function () { - // const mockNavigate = jest.fn(); - // mockUseNavigate.mockReturnValue(mockNavigate); - render( From c6b64faeb352f6b3c275b98b4610933c2730b532 Mon Sep 17 00:00:00 2001 From: nikkikapadia Date: Wed, 11 Dec 2024 14:51:12 -0500 Subject: [PATCH 6/7] add limit to convert functions --- .../dashboards/widgetBuilder/hooks/useWidgetBuilderState.tsx | 1 + .../widgetBuilder/utils/convertBuilderStateToWidget.ts | 1 + .../widgetBuilder/utils/convertWidgetToBuilderStateParams.ts | 1 + 3 files changed, 3 insertions(+) diff --git a/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.tsx b/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.tsx index 4926dd86dd61a0..8f17c11588cc10 100644 --- a/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.tsx +++ b/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.tsx @@ -21,6 +21,7 @@ export type WidgetBuilderStateQueryParams = { description?: string; displayType?: DisplayType; field?: (string | undefined)[]; + limit?: number; query?: string[]; sort?: string[]; title?: string; diff --git a/static/app/views/dashboards/widgetBuilder/utils/convertBuilderStateToWidget.ts b/static/app/views/dashboards/widgetBuilder/utils/convertBuilderStateToWidget.ts index 5ec914b8cc3620..1f3e3961b8a6f8 100644 --- a/static/app/views/dashboards/widgetBuilder/utils/convertBuilderStateToWidget.ts +++ b/static/app/views/dashboards/widgetBuilder/utils/convertBuilderStateToWidget.ts @@ -51,5 +51,6 @@ export function convertBuilderStateToWidget(state: WidgetBuilderState): Widget { interval: '1h', // TODO: Not sure what to put here yet queries: widgetQueries, widgetType: state.dataset, + limit: state.limit, }; } diff --git a/static/app/views/dashboards/widgetBuilder/utils/convertWidgetToBuilderStateParams.ts b/static/app/views/dashboards/widgetBuilder/utils/convertWidgetToBuilderStateParams.ts index 64424e45792cf1..676529168f1805 100644 --- a/static/app/views/dashboards/widgetBuilder/utils/convertWidgetToBuilderStateParams.ts +++ b/static/app/views/dashboards/widgetBuilder/utils/convertWidgetToBuilderStateParams.ts @@ -27,6 +27,7 @@ export function convertWidgetToBuilderStateParams( description: widget.description ?? '', dataset: widget.widgetType ?? WidgetType.ERRORS, displayType: widget.displayType ?? DisplayType.TABLE, + limit: widget.limit, field, yAxis, query, From ceabd830145ec1346793303ee88fb30802360622 Mon Sep 17 00:00:00 2001 From: nikkikapadia Date: Wed, 11 Dec 2024 14:58:26 -0500 Subject: [PATCH 7/7] add limit to tests --- .../utils/convertBuilderStateToWidget.spec.tsx | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/static/app/views/dashboards/widgetBuilder/utils/convertBuilderStateToWidget.spec.tsx b/static/app/views/dashboards/widgetBuilder/utils/convertBuilderStateToWidget.spec.tsx index 563e27c814516a..ab2f226a1a83fa 100644 --- a/static/app/views/dashboards/widgetBuilder/utils/convertBuilderStateToWidget.spec.tsx +++ b/static/app/views/dashboards/widgetBuilder/utils/convertBuilderStateToWidget.spec.tsx @@ -10,6 +10,7 @@ describe('convertBuilderStateToWidget', function () { description: 'Test Description', dataset: WidgetType.ERRORS, displayType: DisplayType.TABLE, + limit: 5, fields: [], yAxis: [], }; @@ -22,6 +23,7 @@ describe('convertBuilderStateToWidget', function () { widgetType: WidgetType.ERRORS, displayType: DisplayType.TABLE, interval: '1h', + limit: 5, queries: [ { ...getDatasetConfig(WidgetType.ERRORS).defaultWidgetQuery, @@ -36,6 +38,7 @@ describe('convertBuilderStateToWidget', function () { description: 'test description for an issues widget', dataset: WidgetType.ISSUE, displayType: DisplayType.TABLE, + limit: 5, fields: [], yAxis: [], }; @@ -48,6 +51,7 @@ describe('convertBuilderStateToWidget', function () { widgetType: WidgetType.ISSUE, displayType: DisplayType.TABLE, interval: '1h', + limit: 5, queries: [ { ...getDatasetConfig(WidgetType.ISSUE).defaultWidgetQuery, @@ -62,6 +66,7 @@ describe('convertBuilderStateToWidget', function () { description: 'Test Description', dataset: WidgetType.ERRORS, displayType: DisplayType.TABLE, + limit: 5, fields: [ {kind: 'field', field: 'geo.country'}, { @@ -84,6 +89,7 @@ describe('convertBuilderStateToWidget', function () { widgetType: WidgetType.ERRORS, displayType: DisplayType.TABLE, interval: '1h', + limit: 5, queries: [ { fields: ['geo.country', 'count()', 'count_unique(user)'],