diff --git a/static/app/utils/url/replaceUrlWithoutNavigation.tsx b/static/app/utils/url/replaceUrlWithoutNavigation.tsx new file mode 100644 index 000000000000..038e19a26cdb --- /dev/null +++ b/static/app/utils/url/replaceUrlWithoutNavigation.tsx @@ -0,0 +1,28 @@ +import * as qs from 'query-string'; + +/** + * Replaces the current browser URL without going through the router. + * + * Updating the URL via `navigate()` notifies the router, which re-renders + * every `useLocation` subscriber (and their subtrees) even when the change is + * only meant to persist state in the URL. This helper writes the URL with + * `history.replaceState` so the address bar, refreshes, and copied links stay + * accurate without triggering any React re-renders. + * + * Trade-off: the router's in-memory location is NOT updated, so + * `useLocation().query` will not reflect params written this way. Only use + * this for params that are exclusively read back via component state (e.g. a + * store hydrated from the URL on mount), never for params other components + * read from the router location. + */ +export function replaceUrlWithoutNavigation(to: { + pathname: string; + query: Record; +}) { + const search = qs.stringify(to.query); + const url = search ? `${to.pathname}?${search}` : to.pathname; + + // Preserve the existing history state so the router's back/forward + // bookkeeping (e.g. react-router's `idx`) stays intact. + window.history.replaceState(window.history.state, '', url); +} diff --git a/static/app/views/dashboards/datasetConfig/traceMetrics.tsx b/static/app/views/dashboards/datasetConfig/traceMetrics.tsx index 64246f9ac45f..db8779b2230a 100644 --- a/static/app/views/dashboards/datasetConfig/traceMetrics.tsx +++ b/static/app/views/dashboards/datasetConfig/traceMetrics.tsx @@ -32,7 +32,7 @@ import { import {formatTraceMetricsFunction} from 'sentry/views/dashboards/datasetConfig/formatTraceMetricsFunction'; import {combineBaseFieldsWithTags} from 'sentry/views/dashboards/datasetConfig/utils/combineBaseFieldsWithEapTags'; import {DisplayType, type WidgetQuery} from 'sentry/views/dashboards/types'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import {useWidgetBuilderStateSlice} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {useTraceMetricMultiMetricSelection} from 'sentry/views/dashboards/widgetBuilder/hooks/useTraceMetricMultiMetricSelection'; import { extractTraceMetricFromColumn, @@ -192,7 +192,7 @@ function useTraceMetricsSearchBarDataProvider( } function useTraceMetricsSearchScope() { - const {state: widgetBuilderState} = useWidgetBuilderContext(); + const widgetBuilderState = useWidgetBuilderStateSlice('displayType', 'fields', 'yAxis'); const hasMultiMetricSelection = useTraceMetricMultiMetricSelection(); const aggregateSource = getTraceMetricAggregateSource( diff --git a/static/app/views/dashboards/widgetBuilder/components/axisRangeSection.tsx b/static/app/views/dashboards/widgetBuilder/components/axisRangeSection.tsx index fc3d265e1b46..9a96791c034e 100644 --- a/static/app/views/dashboards/widgetBuilder/components/axisRangeSection.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/axisRangeSection.tsx @@ -5,12 +5,16 @@ import {Flex} from '@sentry/scraps/layout'; import {t} from 'sentry/locale'; import {getDatasetConfig} from 'sentry/views/dashboards/datasetConfig/base'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import { + useWidgetBuilderDispatch, + useWidgetBuilderStateSlice, +} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {BuilderStateAction} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; export function AxisRangeSection() { const theme = useTheme(); - const {state, dispatch} = useWidgetBuilderContext(); + const state = useWidgetBuilderStateSlice('axisRange', 'dataset'); + const dispatch = useWidgetBuilderDispatch(); const datasetConfig = getDatasetConfig(state.dataset); const value = state.axisRange ?? datasetConfig.axisRange ?? 'auto'; diff --git a/static/app/views/dashboards/widgetBuilder/components/datasetSelector.spec.tsx b/static/app/views/dashboards/widgetBuilder/components/datasetSelector.spec.tsx index 3f664f7ddf96..fe6593e2a23a 100644 --- a/static/app/views/dashboards/widgetBuilder/components/datasetSelector.spec.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/datasetSelector.spec.tsx @@ -2,21 +2,16 @@ import {OrganizationFixture} from 'sentry-fixture/organization'; import {render, screen, userEvent} from 'sentry-test/reactTestingLibrary'; -import {useNavigate} from 'sentry/utils/useNavigate'; +import {replaceUrlWithoutNavigation} from 'sentry/utils/url/replaceUrlWithoutNavigation'; import {WidgetBuilderDatasetSelector as DatasetSelector} from 'sentry/views/dashboards/widgetBuilder/components/datasetSelector'; import {WidgetBuilderProvider} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; -jest.mock('sentry/utils/useNavigate', () => ({ - useNavigate: jest.fn(), -})); +jest.mock('sentry/utils/url/replaceUrlWithoutNavigation'); -const mockUseNavigate = jest.mocked(useNavigate); +const mockReplaceUrl = jest.mocked(replaceUrlWithoutNavigation); describe('DatasetSelector', () => { it('changes the dataset', async () => { - const mockNavigate = jest.fn(); - mockUseNavigate.mockReturnValue(mockNavigate); - render( @@ -27,11 +22,10 @@ describe('DatasetSelector', () => { await userEvent.click(await screen.findByRole('option', {name: 'Issues'})); - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({dataset: 'issue'}), - }), - expect.anything() + }) ); }); @@ -61,9 +55,6 @@ describe('DatasetSelector', () => { }); it('allows selection of transactions dataset when discover-saved-queries-deprecation feature is disabled', async () => { - const mockNavigate = jest.fn(); - mockUseNavigate.mockReturnValue(mockNavigate); - const organizationWithoutDeprecation = OrganizationFixture({ features: [], // No discover-saved-queries-deprecation feature }); @@ -87,11 +78,10 @@ describe('DatasetSelector', () => { await userEvent.click(transactionsOption); - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({dataset: 'transaction-like'}), - }), - expect.anything() + }) ); }); }); diff --git a/static/app/views/dashboards/widgetBuilder/components/datasetSelector.tsx b/static/app/views/dashboards/widgetBuilder/components/datasetSelector.tsx index c2ed0a5f963d..a6be380a277c 100644 --- a/static/app/views/dashboards/widgetBuilder/components/datasetSelector.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/datasetSelector.tsx @@ -11,7 +11,7 @@ import {WidgetBuilderVersion} from 'sentry/utils/analytics/dashboardsAnalyticsEv import {useOrganization} from 'sentry/utils/useOrganization'; import {WidgetType} from 'sentry/views/dashboards/types'; import {SectionHeader} from 'sentry/views/dashboards/widgetBuilder/components/common/sectionHeader'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import {useWidgetBuilderStateSlice} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {useCacheBuilderState} from 'sentry/views/dashboards/widgetBuilder/hooks/useCacheBuilderState'; import {useDashboardWidgetSource} from 'sentry/views/dashboards/widgetBuilder/hooks/useDashboardWidgetSource'; import {useIsEditingWidget} from 'sentry/views/dashboards/widgetBuilder/hooks/useIsEditingWidget'; @@ -19,7 +19,7 @@ import {isLogsEnabled} from 'sentry/views/explore/logs/isLogsEnabled'; export function WidgetBuilderDatasetSelector() { const organization = useOrganization(); - const {state} = useWidgetBuilderContext(); + const state = useWidgetBuilderStateSlice('dataset'); const source = useDashboardWidgetSource(); const isEditing = useIsEditingWidget(); const {cacheBuilderState, restoreOrSetBuilderState} = useCacheBuilderState(); diff --git a/static/app/views/dashboards/widgetBuilder/components/descriptionField.tsx b/static/app/views/dashboards/widgetBuilder/components/descriptionField.tsx index 14491bfae8e3..a640674d38fa 100644 --- a/static/app/views/dashboards/widgetBuilder/components/descriptionField.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/descriptionField.tsx @@ -5,7 +5,10 @@ import {trackAnalytics} from 'sentry/utils/analytics'; import {WidgetBuilderVersion} from 'sentry/utils/analytics/dashboardsAnalyticsEvents'; import {useOrganization} from 'sentry/utils/useOrganization'; import {DisplayType} from 'sentry/views/dashboards/types'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import { + useWidgetBuilderDispatch, + useWidgetBuilderStateSlice, +} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {useDashboardWidgetSource} from 'sentry/views/dashboards/widgetBuilder/hooks/useDashboardWidgetSource'; import {useIsEditingWidget} from 'sentry/views/dashboards/widgetBuilder/hooks/useIsEditingWidget'; import {BuilderStateAction} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; @@ -22,7 +25,13 @@ export function WidgetBuilderDescriptionField({ autosize = true, }: WidgetBuilderDescriptionFieldProps) { const organization = useOrganization(); - const {state, dispatch} = useWidgetBuilderContext(); + const state = useWidgetBuilderStateSlice( + 'dataset', + 'description', + 'displayType', + 'textContent' + ); + const dispatch = useWidgetBuilderDispatch(); const isEditing = useIsEditingWidget(); const source = useDashboardWidgetSource(); const isTextWidget = state.displayType === DisplayType.TEXT; diff --git a/static/app/views/dashboards/widgetBuilder/components/groupBySelector.tsx b/static/app/views/dashboards/widgetBuilder/components/groupBySelector.tsx index 3b5d5a3f172f..0dcc7f123bdf 100644 --- a/static/app/views/dashboards/widgetBuilder/components/groupBySelector.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/groupBySelector.tsx @@ -10,7 +10,10 @@ import {getDatasetConfig} from 'sentry/views/dashboards/datasetConfig/base'; import {WidgetType, type ValidateWidgetResponse} from 'sentry/views/dashboards/types'; import {GroupBySelector} from 'sentry/views/dashboards/widgetBuilder/buildSteps/groupByStep/groupBySelector'; import {SectionHeader} from 'sentry/views/dashboards/widgetBuilder/components/common/sectionHeader'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import { + useWidgetBuilderDispatch, + useWidgetBuilderStateSlice, +} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {useDisableTransactionWidget} from 'sentry/views/dashboards/widgetBuilder/hooks/useDisableTransactionWidget'; import {BuilderStateAction} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; import {useWidgetBuilderTraceItemConfig} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderTraceItemConfig'; @@ -25,7 +28,8 @@ interface WidgetBuilderGroupBySelectorProps { export function WidgetBuilderGroupBySelector({ validatedWidgetResponse, }: WidgetBuilderGroupBySelectorProps) { - const {state, dispatch} = useWidgetBuilderContext(); + const state = useWidgetBuilderStateSlice('dataset', 'fields'); + const dispatch = useWidgetBuilderDispatch(); const disableTransactionWidget = useDisableTransactionWidget(); const organization = useOrganization(); diff --git a/static/app/views/dashboards/widgetBuilder/components/legendTypeSelector.tsx b/static/app/views/dashboards/widgetBuilder/components/legendTypeSelector.tsx index 7cf2d455d371..ba2280f9eecc 100644 --- a/static/app/views/dashboards/widgetBuilder/components/legendTypeSelector.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/legendTypeSelector.tsx @@ -3,11 +3,15 @@ import {Flex} from '@sentry/scraps/layout'; import {Text} from '@sentry/scraps/text'; import {t} from 'sentry/locale'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import { + useWidgetBuilderDispatch, + useWidgetBuilderStateSlice, +} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {BuilderStateAction} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; export function LegendTypeSelector() { - const {state, dispatch} = useWidgetBuilderContext(); + const state = useWidgetBuilderStateSlice('legendType'); + const dispatch = useWidgetBuilderDispatch(); return ( diff --git a/static/app/views/dashboards/widgetBuilder/components/nameAndDescFields.spec.tsx b/static/app/views/dashboards/widgetBuilder/components/nameAndDescFields.spec.tsx index e8ad81123dd6..6c2642534602 100644 --- a/static/app/views/dashboards/widgetBuilder/components/nameAndDescFields.spec.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/nameAndDescFields.spec.tsx @@ -8,7 +8,7 @@ describe('WidgetBuilder', () => { it('edits name and description', async () => { setWindowLocation('http://localhost/organizations/org-slug/dashboard/1/?project=-1'); - const {router} = render( + render( @@ -18,11 +18,9 @@ describe('WidgetBuilder', () => { // trigger blur await userEvent.tab(); - expect(router.location).toEqual( - expect.objectContaining({ - query: expect.objectContaining({title: 'some name'}), - }) - ); + // The widget builder writes its params to the URL without notifying the + // router, so assert against the browser URL + expect(window.location.search).toContain('title=some%20name'); await userEvent.click(await screen.findByTestId('add-description')); @@ -33,11 +31,7 @@ describe('WidgetBuilder', () => { // trigger blur await userEvent.tab(); - expect(router.location).toEqual( - expect.objectContaining({ - query: expect.objectContaining({description: 'some description'}), - }) - ); + expect(window.location.search).toContain('description=some%20description'); }); it('displays error', async () => { diff --git a/static/app/views/dashboards/widgetBuilder/components/nameAndDescFields.tsx b/static/app/views/dashboards/widgetBuilder/components/nameAndDescFields.tsx index 51b2920ddec4..ccfc7490388f 100644 --- a/static/app/views/dashboards/widgetBuilder/components/nameAndDescFields.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/nameAndDescFields.tsx @@ -11,7 +11,10 @@ import {useOrganization} from 'sentry/utils/useOrganization'; import {DisplayType} from 'sentry/views/dashboards/types'; import {SectionHeader} from 'sentry/views/dashboards/widgetBuilder/components/common/sectionHeader'; import {WidgetBuilderDescriptionField} from 'sentry/views/dashboards/widgetBuilder/components/descriptionField'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import { + useWidgetBuilderDispatch, + useWidgetBuilderStateSlice, +} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {useDashboardWidgetSource} from 'sentry/views/dashboards/widgetBuilder/hooks/useDashboardWidgetSource'; import {useIsEditingWidget} from 'sentry/views/dashboards/widgetBuilder/hooks/useIsEditingWidget'; import {BuilderStateAction} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; @@ -26,7 +29,13 @@ export function WidgetBuilderNameAndDescription({ setError, }: WidgetBuilderNameAndDescriptionProps) { const organization = useOrganization(); - const {state, dispatch} = useWidgetBuilderContext(); + const state = useWidgetBuilderStateSlice( + 'title', + 'description', + 'displayType', + 'dataset' + ); + const dispatch = useWidgetBuilderDispatch(); const [isDescSelected, setIsDescSelected] = useState(state.description ? true : false); const isEditing = useIsEditingWidget(); const source = useDashboardWidgetSource(); diff --git a/static/app/views/dashboards/widgetBuilder/components/newWidgetBuilder.tsx b/static/app/views/dashboards/widgetBuilder/components/newWidgetBuilder.tsx index 4dc8e606c3a6..6221a7b35fa8 100644 --- a/static/app/views/dashboards/widgetBuilder/components/newWidgetBuilder.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/newWidgetBuilder.tsx @@ -10,7 +10,6 @@ import {closestCorners, DndContext, useDraggable, useDroppable} from '@dnd-kit/c import {css, Global, useTheme} from '@emotion/react'; import styled from '@emotion/styled'; import {AnimatePresence, motion, type MotionNodeAnimationOptions} from 'framer-motion'; -import omit from 'lodash/omit'; import {Backdrop} from '@sentry/scraps/backdrop'; import {Flex} from '@sentry/scraps/layout'; @@ -18,6 +17,7 @@ import {Flex} from '@sentry/scraps/layout'; import {usePageFilters} from 'sentry/components/pageFilters/usePageFilters'; import {t} from 'sentry/locale'; import {CustomMeasurementsProvider} from 'sentry/utils/customMeasurements/customMeasurementsProvider'; +import {defined} from 'sentry/utils/defined'; import {EventView} from 'sentry/utils/discover/eventView'; import {MetricsCardinalityProvider} from 'sentry/utils/performance/contexts/metricsCardinality'; import {MEPSettingProvider} from 'sentry/utils/performance/contexts/metricsEnhancedSetting'; @@ -46,7 +46,8 @@ import {WidgetBuilderFilterBar} from 'sentry/views/dashboards/widgetBuilder/comp import {WidgetBuilderSlideout} from 'sentry/views/dashboards/widgetBuilder/components/widgetBuilderSlideout'; import {WidgetPreview} from 'sentry/views/dashboards/widgetBuilder/components/widgetPreview'; import { - useWidgetBuilderContext, + useWidgetBuilderStateSlice, + useWidgetBuilderUrlParams, WidgetBuilderProvider, } from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import type {OnDataFetchedParams} from 'sentry/views/dashboards/widgetCard'; @@ -248,7 +249,8 @@ export function WidgetPreviewContainer({ onDataFetched?: (results: OnDataFetchedParams) => void; openWidgetTemplates?: boolean; }) { - const {state} = useWidgetBuilderContext(); + const state = useWidgetBuilderStateSlice('displayType'); + const builderUrlParams = useWidgetBuilderUrlParams(); const organization = useOrganization(); const location = useLocation(); const theme = useTheme(); @@ -283,18 +285,14 @@ export function WidgetPreviewContainer({ position: isDragEnabled ? 'fixed' : undefined, }; - // check if the state is in the url because the state variable has default values - const hasUrlParams = - Object.keys( - omit(location.query, [ - 'environment', - 'project', - 'release', - 'start', - 'end', - 'statsPeriod', - ]) - ).length > 0; + // check if the state is in the url because the state variable has default + // values. The store writes its params to the URL without notifying the + // router, so merge them over the router's (possibly stale) query. + const hasUrlParams = Object.entries({...location.query, ...builderUrlParams}).some( + ([key, value]) => + defined(value) && + !['environment', 'project', 'release', 'start', 'end', 'statsPeriod'].includes(key) + ); const getPreviewHeight = () => { if (isDragEnabled) { diff --git a/static/app/views/dashboards/widgetBuilder/components/queryFilterBuilder.tsx b/static/app/views/dashboards/widgetBuilder/components/queryFilterBuilder.tsx index 531ed3dea517..16b2106afc93 100644 --- a/static/app/views/dashboards/widgetBuilder/components/queryFilterBuilder.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/queryFilterBuilder.tsx @@ -25,7 +25,10 @@ import { } from 'sentry/views/dashboards/types'; import {SectionHeader} from 'sentry/views/dashboards/widgetBuilder/components/common/sectionHeader'; import {WidgetOnDemandQueryWarning} from 'sentry/views/dashboards/widgetBuilder/components/widgetOnDemandQueryWarning'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import { + useWidgetBuilderDispatch, + useWidgetBuilderQueryState, +} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {useDashboardWidgetSource} from 'sentry/views/dashboards/widgetBuilder/hooks/useDashboardWidgetSource'; import {useDisableTransactionWidget} from 'sentry/views/dashboards/widgetBuilder/hooks/useDisableTransactionWidget'; import {useIsEditingWidget} from 'sentry/views/dashboards/widgetBuilder/hooks/useIsEditingWidget'; @@ -42,7 +45,8 @@ export function WidgetBuilderQueryFilterBuilder({ onQueryConditionChange, validatedWidgetResponse, }: WidgetBuilderQueryFilterBuilderProps) { - const {state, dispatch} = useWidgetBuilderContext(); + const state = useWidgetBuilderQueryState(); + const dispatch = useWidgetBuilderDispatch(); const {selection} = usePageFilters(); const organization = useOrganization(); const [queryConditionValidity, setQueryConditionValidity] = useState(() => { diff --git a/static/app/views/dashboards/widgetBuilder/components/saveButton.tsx b/static/app/views/dashboards/widgetBuilder/components/saveButton.tsx index fd17be135195..244ca9db3d57 100644 --- a/static/app/views/dashboards/widgetBuilder/components/saveButton.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/saveButton.tsx @@ -14,7 +14,7 @@ import {useOrganization} from 'sentry/utils/useOrganization'; import {useParams} from 'sentry/utils/useParams'; import {type Widget, WidgetType} from 'sentry/views/dashboards/types'; import {flattenErrors} from 'sentry/views/dashboards/utils'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import {useWidgetBuilderStore} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {useDisableTransactionWidget} from 'sentry/views/dashboards/widgetBuilder/hooks/useDisableTransactionWidget'; import {convertBuilderStateToWidget} from 'sentry/views/dashboards/widgetBuilder/utils/convertBuilderStateToWidget'; @@ -25,7 +25,9 @@ export interface SaveButtonProps { } export function SaveButton({isEditing, onSave, setError}: SaveButtonProps) { - const {state} = useWidgetBuilderContext(); + // The state is only needed when the button is clicked, so read it from the + // store on demand instead of subscribing to every state change + const store = useWidgetBuilderStore(); const {widgetIndex} = useParams(); const api = useApi(); const organization = useOrganization(); @@ -33,6 +35,7 @@ export function SaveButton({isEditing, onSave, setError}: SaveButtonProps) { const disableTransactionWidget = useDisableTransactionWidget(); const handleSave = async () => { + const state = store.getState(); trackAnalytics('dashboards_views.widget_builder.save', { builder_version: WidgetBuilderVersion.SLIDEOUT, data_set: state.dataset ?? '', diff --git a/static/app/views/dashboards/widgetBuilder/components/sortBySelector.spec.tsx b/static/app/views/dashboards/widgetBuilder/components/sortBySelector.spec.tsx index 4d14e0e3d117..3cef79f0fa68 100644 --- a/static/app/views/dashboards/widgetBuilder/components/sortBySelector.spec.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/sortBySelector.spec.tsx @@ -5,15 +5,13 @@ import type {RouterConfig} from 'sentry-test/reactTestingLibrary'; import type {Organization} from 'sentry/types/organization'; import {ELLIPSIS} from 'sentry/utils/string/unicode'; -import {useNavigate} from 'sentry/utils/useNavigate'; +import {replaceUrlWithoutNavigation} from 'sentry/utils/url/replaceUrlWithoutNavigation'; import {WidgetBuilderSortBySelector} from 'sentry/views/dashboards/widgetBuilder/components/sortBySelector'; import {WidgetBuilderProvider} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; -jest.mock('sentry/utils/useNavigate', () => ({ - useNavigate: jest.fn(), -})); +jest.mock('sentry/utils/url/replaceUrlWithoutNavigation'); -const mockUseNavigate = jest.mocked(useNavigate); +const mockReplaceUrl = jest.mocked(replaceUrlWithoutNavigation); describe('WidgetBuilderSortBySelector', () => { let organization: Organization; @@ -99,9 +97,6 @@ describe('WidgetBuilderSortBySelector', () => { }); it('renders and functions correctly', async () => { - const mockNavigate = jest.fn(); - mockUseNavigate.mockReturnValue(mockNavigate); - render( @@ -120,20 +115,18 @@ describe('WidgetBuilderSortBySelector', () => { await userEvent.click(sortFieldSelector); await userEvent.click(await screen.findByText('count()')); - expect(mockNavigate).toHaveBeenLastCalledWith( + expect(mockReplaceUrl).toHaveBeenLastCalledWith( expect.objectContaining({ query: expect.objectContaining({sort: ['-count()']}), - }), - expect.anything() + }) ); await userEvent.click(sortDirectionSelector); await userEvent.click(await screen.findByText('Low to high')); - expect(mockNavigate).toHaveBeenLastCalledWith( + expect(mockReplaceUrl).toHaveBeenLastCalledWith( expect.objectContaining({ query: expect.objectContaining({sort: ['count()']}), - }), - expect.anything() + }) ); }); @@ -175,9 +168,6 @@ describe('WidgetBuilderSortBySelector', () => { }); it('correctly handles limit changes', async () => { - const mockNavigate = jest.fn(); - mockUseNavigate.mockReturnValue(mockNavigate); - render( @@ -192,17 +182,14 @@ describe('WidgetBuilderSortBySelector', () => { await userEvent.click(limitSelector); await userEvent.click(await screen.findByText('Limit to 3 results')); - expect(mockNavigate).toHaveBeenLastCalledWith( + expect(mockReplaceUrl).toHaveBeenLastCalledWith( expect.objectContaining({ query: expect.objectContaining({limit: 3}), - }), - expect.anything() + }) ); }); it('switches the default value for count_unique functions', async () => { - const mockNavigate = jest.fn(); - mockUseNavigate.mockReturnValue(mockNavigate); MockApiClient.addMockResponse({ url: '/organizations/org-slug/trace-items/attributes/', body: [{key: 'span.duration', name: 'span.duration'}], @@ -240,18 +227,14 @@ describe('WidgetBuilderSortBySelector', () => { await userEvent.click(screen.getByText(`count(${ELLIPSIS})`)); await userEvent.click(screen.getByText(`count_unique(${ELLIPSIS})`)); - expect(mockNavigate).toHaveBeenLastCalledWith( + expect(mockReplaceUrl).toHaveBeenLastCalledWith( expect.objectContaining({ query: expect.objectContaining({sort: ['-count_unique(span.op)']}), - }), - expect.anything() + }) ); }); it('sorts by equations line chart', async () => { - const mockNavigate = jest.fn(); - mockUseNavigate.mockReturnValue(mockNavigate); - const organizationWithFlag = OrganizationFixture({ features: ['open-membership', 'visibility-explore-view'], }); @@ -285,20 +268,18 @@ describe('WidgetBuilderSortBySelector', () => { await screen.findByText('count_unique(transaction.duration) + 100') ); - expect(mockNavigate).toHaveBeenLastCalledWith( + expect(mockReplaceUrl).toHaveBeenLastCalledWith( expect.objectContaining({ query: expect.objectContaining({sort: ['-equation[0]']}), - }), - expect.anything() + }) ); await userEvent.click(sortDirectionSelector); await userEvent.click(await screen.findByText('Low to high')); - expect(mockNavigate).toHaveBeenLastCalledWith( + expect(mockReplaceUrl).toHaveBeenLastCalledWith( expect.objectContaining({ query: expect.objectContaining({sort: ['equation[0]']}), - }), - expect.anything() + }) ); }); it('renders a limit selector for categorical bar widgets', async () => { @@ -351,9 +332,6 @@ describe('WidgetBuilderSortBySelector', () => { }); it('correctly handles categorical bar limit changes', async () => { - const mockNavigate = jest.fn(); - mockUseNavigate.mockReturnValue(mockNavigate); - render( @@ -379,18 +357,14 @@ describe('WidgetBuilderSortBySelector', () => { await userEvent.click(limitSelector); await userEvent.click(await screen.findByText('Limit to 15 results')); - expect(mockNavigate).toHaveBeenLastCalledWith( + expect(mockReplaceUrl).toHaveBeenLastCalledWith( expect.objectContaining({ query: expect.objectContaining({limit: 15}), - }), - expect.anything() + }) ); }); it('sorts by equations table', async () => { - const mockNavigate = jest.fn(); - mockUseNavigate.mockReturnValue(mockNavigate); - const organizationWithFlag = OrganizationFixture({ features: ['open-membership', 'visibility-explore-view'], }); @@ -425,20 +399,18 @@ describe('WidgetBuilderSortBySelector', () => { await screen.findByText('count_unique(transaction.duration) + 100') ); - expect(mockNavigate).toHaveBeenLastCalledWith( + expect(mockReplaceUrl).toHaveBeenLastCalledWith( expect.objectContaining({ query: expect.objectContaining({sort: ['-equation[0]']}), - }), - expect.anything() + }) ); await userEvent.click(sortDirectionSelector); await userEvent.click(await screen.findByText('Low to high')); - expect(mockNavigate).toHaveBeenLastCalledWith( + expect(mockReplaceUrl).toHaveBeenLastCalledWith( expect.objectContaining({ query: expect.objectContaining({sort: ['equation[0]']}), - }), - expect.anything() + }) ); }); }); diff --git a/static/app/views/dashboards/widgetBuilder/components/sortBySelector.tsx b/static/app/views/dashboards/widgetBuilder/components/sortBySelector.tsx index 5993535d8921..22a5deb7805d 100644 --- a/static/app/views/dashboards/widgetBuilder/components/sortBySelector.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/sortBySelector.tsx @@ -19,7 +19,10 @@ import { } from 'sentry/views/dashboards/types'; import {SectionHeader} from 'sentry/views/dashboards/widgetBuilder/components/common/sectionHeader'; import {SortBySelectors} from 'sentry/views/dashboards/widgetBuilder/components/sortBySelectors'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import { + useWidgetBuilderDispatch, + useWidgetBuilderQueryState, +} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {useDashboardWidgetSource} from 'sentry/views/dashboards/widgetBuilder/hooks/useDashboardWidgetSource'; import {useIsEditingWidget} from 'sentry/views/dashboards/widgetBuilder/hooks/useIsEditingWidget'; import {BuilderStateAction} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; @@ -33,7 +36,8 @@ import {useTraceItemDatasetAttributes} from 'sentry/views/explore/hooks/useTrace import {HiddenTraceMetricGroupByFields} from 'sentry/views/explore/metrics/constants'; export function WidgetBuilderSortBySelector() { - const {state, dispatch} = useWidgetBuilderContext(); + const state = useWidgetBuilderQueryState(); + const dispatch = useWidgetBuilderDispatch(); const widget = convertBuilderStateToWidget(state); const organization = useOrganization(); const source = useDashboardWidgetSource(); diff --git a/static/app/views/dashboards/widgetBuilder/components/thresholds.spec.tsx b/static/app/views/dashboards/widgetBuilder/components/thresholds.spec.tsx index 8eeba55d4695..4442cab4b1bc 100644 --- a/static/app/views/dashboards/widgetBuilder/components/thresholds.spec.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/thresholds.spec.tsx @@ -1,20 +1,17 @@ import {render, screen, userEvent} from 'sentry-test/reactTestingLibrary'; -import {useNavigate} from 'sentry/utils/useNavigate'; +import {replaceUrlWithoutNavigation} from 'sentry/utils/url/replaceUrlWithoutNavigation'; import {ThresholdsSection as Thresholds} from 'sentry/views/dashboards/widgetBuilder/components/thresholds'; import { useWidgetBuilderContext, WidgetBuilderProvider, } from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; -jest.mock('sentry/utils/useNavigate'); -describe('Thresholds', () => { - let mockNavigate!: jest.Mock; +jest.mock('sentry/utils/url/replaceUrlWithoutNavigation'); - beforeEach(() => { - mockNavigate = jest.fn(); - jest.mocked(useNavigate).mockReturnValue(mockNavigate); - }); +const mockReplaceUrl = jest.mocked(replaceUrlWithoutNavigation); +describe('Thresholds', () => { + beforeEach(() => {}); it('sets thresholds to undefined if the thresholds are fully wiped', async () => { render( @@ -35,13 +32,12 @@ describe('Thresholds', () => { await userEvent.clear(screen.getByLabelText('First Maximum')); - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ thresholds: undefined, }), - }), - expect.anything() + }) ); }); @@ -56,13 +52,12 @@ describe('Thresholds', () => { await userEvent.type(screen.getByLabelText('Second Maximum'), '200'); await userEvent.tab(); - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ thresholds: '{"max_values":{"max1":100,"max2":200},"unit":null}', }), - }), - expect.anything() + }) ); }); @@ -86,13 +81,12 @@ describe('Thresholds', () => { await userEvent.click(screen.getAllByText('millisecond')[0]!); await userEvent.click(screen.getByText('second')); - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ thresholds: '{"max_values":{"max1":100,"max2":200},"unit":"second"}', }), - }), - expect.anything() + }) ); }); @@ -134,13 +128,12 @@ describe('Thresholds', () => { expect((await screen.findAllByDisplayValue('0.5'))[0]).toBeInTheDocument(); expect((await screen.findAllByDisplayValue('100.5456'))[0]).toBeInTheDocument(); - expect(mockNavigate).toHaveBeenLastCalledWith( + expect(mockReplaceUrl).toHaveBeenLastCalledWith( expect.objectContaining({ query: expect.objectContaining({ thresholds: '{"max_values":{"max1":0.5,"max2":100.5456},"unit":null}', }), - }), - expect.anything() + }) ); }); diff --git a/static/app/views/dashboards/widgetBuilder/components/thresholds.tsx b/static/app/views/dashboards/widgetBuilder/components/thresholds.tsx index 9b7ad166b3a5..4ec45bc620fe 100644 --- a/static/app/views/dashboards/widgetBuilder/components/thresholds.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/thresholds.tsx @@ -8,7 +8,10 @@ import { Thresholds, } from 'sentry/views/dashboards/widgetBuilder/buildSteps/thresholdsStep/thresholds'; import {SectionHeader} from 'sentry/views/dashboards/widgetBuilder/components/common/sectionHeader'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import { + useWidgetBuilderDispatch, + useWidgetBuilderStateSlice, +} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {BuilderStateAction} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; type ThresholdsSectionProps = { @@ -24,7 +27,8 @@ export function ThresholdsSection({ error, setError, }: ThresholdsSectionProps) { - const {state, dispatch} = useWidgetBuilderContext(); + const state = useWidgetBuilderStateSlice('thresholds'); + const dispatch = useWidgetBuilderDispatch(); const prevDataTypeRef = useRef(undefined); useEffect(() => { diff --git a/static/app/views/dashboards/widgetBuilder/components/typeSelector.spec.tsx b/static/app/views/dashboards/widgetBuilder/components/typeSelector.spec.tsx index 06b315021e59..dcf2e9d0fc01 100644 --- a/static/app/views/dashboards/widgetBuilder/components/typeSelector.spec.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/typeSelector.spec.tsx @@ -2,22 +2,17 @@ import {OrganizationFixture} from 'sentry-fixture/organization'; import {render, screen, userEvent} from 'sentry-test/reactTestingLibrary'; -import {useNavigate} from 'sentry/utils/useNavigate'; +import {replaceUrlWithoutNavigation} from 'sentry/utils/url/replaceUrlWithoutNavigation'; import {WidgetType} from 'sentry/views/dashboards/types'; import {WidgetBuilderTypeSelector as TypeSelector} from 'sentry/views/dashboards/widgetBuilder/components/typeSelector'; import {WidgetBuilderProvider} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; -jest.mock('sentry/utils/useNavigate', () => ({ - useNavigate: jest.fn(), -})); +jest.mock('sentry/utils/url/replaceUrlWithoutNavigation'); -const mockUseNavigate = jest.mocked(useNavigate); +const mockReplaceUrl = jest.mocked(replaceUrlWithoutNavigation); describe('TypeSelector', () => { it('changes the visualization type', async () => { - const mockNavigate = jest.fn(); - mockUseNavigate.mockReturnValue(mockNavigate); - render( @@ -29,11 +24,10 @@ describe('TypeSelector', () => { // select new option await userEvent.click(await screen.findByText('Bar (Time Series)')); - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({displayType: 'bar'}), - }), - expect.anything() + }) ); }); @@ -48,8 +42,6 @@ describe('TypeSelector', () => { }); it('shows text widget option', async () => { - mockUseNavigate.mockReturnValue(jest.fn()); - render( @@ -64,9 +56,6 @@ describe('TypeSelector', () => { }); it('resets the widget builder state when the display type is changed on an issue widget', async () => { - const mockNavigate = jest.fn(); - mockUseNavigate.mockReturnValue(mockNavigate); - render( @@ -84,36 +73,30 @@ describe('TypeSelector', () => { await userEvent.click(await screen.findByText('Line')); await userEvent.click(await screen.findByText('Table')); - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ displayType: 'table', }), - }), - expect.anything() + }) ); - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ dataset: WidgetType.ISSUE, }), - }), - expect.anything() + }) ); - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ field: ['issue', 'assignee', 'title'], }), - }), - expect.anything() + }) ); }); it('resets the widget builder state to dataset defaults when display type is changed from text widget', async () => { - const mockNavigate = jest.fn(); - mockUseNavigate.mockReturnValue(mockNavigate); - render( @@ -132,29 +115,26 @@ describe('TypeSelector', () => { await userEvent.click(await screen.findByText('Text (Markdown)')); await userEvent.click(await screen.findByText('Table')); - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ displayType: 'table', }), - }), - expect.anything() + }) ); - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ dataset: WidgetType.ERRORS, }), - }), - expect.anything() + }) ); - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ field: ['count_unique(user)'], }), - }), - expect.anything() + }) ); }); }); diff --git a/static/app/views/dashboards/widgetBuilder/components/typeSelector.tsx b/static/app/views/dashboards/widgetBuilder/components/typeSelector.tsx index c073d0ee1f6d..b22357e4edae 100644 --- a/static/app/views/dashboards/widgetBuilder/components/typeSelector.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/typeSelector.tsx @@ -13,7 +13,11 @@ import {getDatasetConfig} from 'sentry/views/dashboards/datasetConfig/base'; import {DisplayType, WidgetType} from 'sentry/views/dashboards/types'; import {usesTimeSeriesData} from 'sentry/views/dashboards/utils'; import {SectionHeader} from 'sentry/views/dashboards/widgetBuilder/components/common/sectionHeader'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import { + useWidgetBuilderDispatch, + useWidgetBuilderStateSlice, + useWidgetBuilderStore, +} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {useDashboardWidgetSource} from 'sentry/views/dashboards/widgetBuilder/hooks/useDashboardWidgetSource'; import {useIsEditingWidget} from 'sentry/views/dashboards/widgetBuilder/hooks/useIsEditingWidget'; import {BuilderStateAction} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; @@ -39,7 +43,9 @@ export function WidgetBuilderTypeSelector({ error, setError, }: WidgetBuilderTypeSelectorProps) { - const {state, dispatch} = useWidgetBuilderContext(); + const state = useWidgetBuilderStateSlice('dataset', 'displayType', 'query'); + const dispatch = useWidgetBuilderDispatch(); + const store = useWidgetBuilderStore(); const config = getDatasetConfig(state.dataset); const source = useDashboardWidgetSource(); const isEditing = useIsEditingWidget(); @@ -123,7 +129,7 @@ export function WidgetBuilderTypeSelector({ ], displayType: newValue, interval: '', - title: state.title ?? '', + title: store.getState().title ?? '', }), }); } @@ -142,7 +148,7 @@ export function WidgetBuilderTypeSelector({ ], displayType: newValue, interval: '', - title: state.title ?? '', + title: store.getState().title ?? '', }), }); }; diff --git a/static/app/views/dashboards/widgetBuilder/components/visualize/index.spec.tsx b/static/app/views/dashboards/widgetBuilder/components/visualize/index.spec.tsx index ba877203e3fa..e9ae2a263c76 100644 --- a/static/app/views/dashboards/widgetBuilder/components/visualize/index.spec.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/visualize/index.spec.tsx @@ -10,8 +10,8 @@ import { import type {TagCollection} from 'sentry/types/group'; import {FieldKind} from 'sentry/utils/fields'; +import {replaceUrlWithoutNavigation} from 'sentry/utils/url/replaceUrlWithoutNavigation'; import {useCustomMeasurements} from 'sentry/utils/useCustomMeasurements'; -import {useNavigate} from 'sentry/utils/useNavigate'; import {DisplayType, WidgetType} from 'sentry/views/dashboards/types'; import {Visualize} from 'sentry/views/dashboards/widgetBuilder/components/visualize'; import {WidgetBuilderProvider} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; @@ -19,7 +19,9 @@ import {useTraceItemDatasetAttributes} from 'sentry/views/explore/hooks/useTrace jest.mock('sentry/utils/useCustomMeasurements'); jest.mock('sentry/views/explore/hooks/useTraceItemAttributes'); -jest.mock('sentry/utils/useNavigate'); +jest.mock('sentry/utils/url/replaceUrlWithoutNavigation'); + +const mockReplaceUrl = jest.mocked(replaceUrlWithoutNavigation); const DASHBOARD_WIDGET_BUILDER_PATHNAME = '/organizations/org-slug/dashboards/new/widget/new/'; @@ -27,7 +29,6 @@ const DASHBOARD_WIDGET_BUILDER_ROUTE = '/organizations/:orgId/dashboards/new/wid describe('Visualize', () => { let organization!: ReturnType; - let mockNavigate!: jest.Mock; beforeEach(() => { organization = OrganizationFixture({ @@ -87,9 +88,6 @@ describe('Visualize', () => { isLoading: false, }; }); - - mockNavigate = jest.fn(); - jest.mocked(useNavigate).mockReturnValue(mockNavigate); }); afterEach(() => { @@ -536,9 +534,8 @@ describe('Visualize', () => { expect(screen.getByRole('button', {name: 'Aggregate Selection'})).toHaveTextContent( 'count' ); - expect(mockNavigate).toHaveBeenCalledWith( - expect.objectContaining({query: expect.objectContaining({field: ['count()']})}), - expect.anything() + expect(mockReplaceUrl).toHaveBeenCalledWith( + expect.objectContaining({query: expect.objectContaining({field: ['count()']})}) ); }); @@ -573,11 +570,10 @@ describe('Visualize', () => { 'count_miserable' ); expect(screen.getByDisplayValue('300')).toBeInTheDocument(); - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({field: ['count_miserable(user,300)']}), - }), - expect.anything() + }) ); }); @@ -922,11 +918,10 @@ describe('Visualize', () => { // The second field is now selected, but the URL param for selectedAggregate // is cleared, so the last field is selected expect(await screen.findByRole('radio', {name: 'field1'})).toBeChecked(); - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({selectedAggregate: undefined}), - }), - expect.anything() + }) ); }); @@ -1531,11 +1526,10 @@ describe('Visualize', () => { }); await userEvent.type(input, '{ArrowDown}{Enter}'); - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({field: ['equation|( avg(span.duration)']}), - }), - expect.anything() + }) ); }); @@ -1582,11 +1576,10 @@ describe('Visualize', () => { }); await userEvent.type(input, '{ArrowDown}{Enter}'); - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({field: ['equation|( avg(span.duration)']}), - }), - expect.anything() + }) ); }); }); diff --git a/static/app/views/dashboards/widgetBuilder/components/visualize/index.tsx b/static/app/views/dashboards/widgetBuilder/components/visualize/index.tsx index 168cafa09ff9..215dc9367d47 100644 --- a/static/app/views/dashboards/widgetBuilder/components/visualize/index.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/visualize/index.tsx @@ -51,7 +51,10 @@ import { import {MetricSelectRow} from 'sentry/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricSelectRow'; import {MetricsEquationVisualize} from 'sentry/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricsEquationVisualize'; import {VisualizeGhostField} from 'sentry/views/dashboards/widgetBuilder/components/visualize/visualizeGhostField'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import { + useWidgetBuilderDispatch, + useWidgetBuilderStateSlice, +} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {useDashboardWidgetSource} from 'sentry/views/dashboards/widgetBuilder/hooks/useDashboardWidgetSource'; import {useDisableTransactionWidget} from 'sentry/views/dashboards/widgetBuilder/hooks/useDisableTransactionWidget'; import {useIsEditingWidget} from 'sentry/views/dashboards/widgetBuilder/hooks/useIsEditingWidget'; @@ -289,7 +292,14 @@ export function Visualize({error, setError, traceMetricsVisualizeMode}: Visualiz const [activeId, setActiveId] = useState(null); const organization = useOrganization(); const theme = useTheme(); - const {state, dispatch} = useWidgetBuilderContext(); + const state = useWidgetBuilderStateSlice( + 'dataset', + 'displayType', + 'fields', + 'selectedAggregate', + 'yAxis' + ); + const dispatch = useWidgetBuilderDispatch(); const tags = useTags(); const {customMeasurements} = useCustomMeasurements(); const source = useDashboardWidgetSource(); diff --git a/static/app/views/dashboards/widgetBuilder/components/visualize/selectRow.tsx b/static/app/views/dashboards/widgetBuilder/components/visualize/selectRow.tsx index 45009c44f90a..e608e3200c89 100644 --- a/static/app/views/dashboards/widgetBuilder/components/visualize/selectRow.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/visualize/selectRow.tsx @@ -29,7 +29,10 @@ import { parseAggregateFromValueKey, PrimarySelectRow, } from 'sentry/views/dashboards/widgetBuilder/components/visualize'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import { + useWidgetBuilderDispatch, + useWidgetBuilderStateSlice, +} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {BuilderStateAction} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; import type {FieldValueOption} from 'sentry/views/discover/table/queryField'; import type {FieldValue} from 'sentry/views/discover/table/types'; @@ -123,7 +126,8 @@ export function SelectRow({ disabled, }: SelectRowProps) { const organization = useOrganization(); - const {state, dispatch} = useWidgetBuilderContext(); + const state = useWidgetBuilderStateSlice('dataset', 'displayType'); + const dispatch = useWidgetBuilderDispatch(); const datasetConfig = getDatasetConfig(state.dataset); const columnSelectRef = useRef(null); diff --git a/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/aggregateSelector.tsx b/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/aggregateSelector.tsx index e8dd94e2a63a..ba69b30772ae 100644 --- a/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/aggregateSelector.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/aggregateSelector.tsx @@ -10,7 +10,10 @@ import type { } from 'sentry/utils/discover/fields'; import {AggregateCompactSelect} from 'sentry/views/dashboards/widgetBuilder/components/visualize'; import {sortSelectedFirst} from 'sentry/views/dashboards/widgetBuilder/components/visualize/selectRow'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import { + useWidgetBuilderDispatch, + useWidgetBuilderStateSlice, +} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import { buildTraceMetricAggregate, getTraceMetricAggregateActionType, @@ -30,7 +33,8 @@ export function AggregateSelector({ index: number; traceMetric: TraceMetric; }) { - const {state, dispatch} = useWidgetBuilderContext(); + const state = useWidgetBuilderStateSlice('displayType', 'fields', 'yAxis'); + const dispatch = useWidgetBuilderDispatch(); const aggregateSource = getTraceMetricAggregateSource( state.displayType, diff --git a/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricSelectRow.spec.tsx b/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricSelectRow.spec.tsx index 9bba01b90d59..a4f723f71461 100644 --- a/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricSelectRow.spec.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricSelectRow.spec.tsx @@ -4,25 +4,21 @@ import type { AggregationKeyWithAlias, QueryFieldValue, } from 'sentry/utils/discover/fields'; -import {useNavigate} from 'sentry/utils/useNavigate'; +import {replaceUrlWithoutNavigation} from 'sentry/utils/url/replaceUrlWithoutNavigation'; import {DisplayType, WidgetType} from 'sentry/views/dashboards/types'; import {MetricSelectRow} from 'sentry/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricSelectRow'; import {WidgetBuilderProvider} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {serializeFields} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; import {FieldValueKind} from 'sentry/views/discover/table/types'; -jest.mock('sentry/utils/useNavigate'); -const mockedUseNavigate = jest.mocked(useNavigate); +jest.mock('sentry/utils/url/replaceUrlWithoutNavigation'); +const mockReplaceUrl = jest.mocked(replaceUrlWithoutNavigation); const DASHBOARD_WIDGET_BUILDER_PATHNAME = '/organizations/org-slug/dashboards/new/widget/new/'; describe('MetricSelectRow', () => { - let mockNavigate!: jest.Mock; beforeEach(() => { - mockNavigate = jest.fn(); - mockedUseNavigate.mockReturnValue(mockNavigate); - MockApiClient.addMockResponse({ url: '/organizations/org-slug/trace-items/attributes/', method: 'GET', @@ -216,7 +212,7 @@ describe('MetricSelectRow', () => { // p50 is invalid for counter, so it should be replaced with sum await waitFor(() => { - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ yAxis: serializeFields([ @@ -226,8 +222,7 @@ describe('MetricSelectRow', () => { }, ]), }), - }), - expect.anything() + }) ); }); }); @@ -268,7 +263,7 @@ describe('MetricSelectRow', () => { // sum should remain since it's valid for distribution, but args updated await waitFor(() => { - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ yAxis: serializeFields([ @@ -278,8 +273,7 @@ describe('MetricSelectRow', () => { }, ]), }), - }), - expect.anything() + }) ); }); }); @@ -324,7 +318,7 @@ describe('MetricSelectRow', () => { // per_second stays, p99 replaced with sum, count replaced with sum await waitFor(() => { - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ yAxis: serializeFields([ @@ -348,8 +342,7 @@ describe('MetricSelectRow', () => { }, ]), }), - }), - expect.anything() + }) ); }); }); @@ -388,7 +381,7 @@ describe('MetricSelectRow', () => { // avg is invalid for counter, replaced with sum await waitFor(() => { - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ field: serializeFields([ @@ -398,8 +391,7 @@ describe('MetricSelectRow', () => { }, ]), }), - }), - expect.anything() + }) ); }); }); @@ -440,7 +432,7 @@ describe('MetricSelectRow', () => { // p50 is invalid for counter, replaced with avg await waitFor(() => { - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ yAxis: serializeFields([ @@ -450,8 +442,7 @@ describe('MetricSelectRow', () => { }, ]), }), - }), - expect.anything() + }) ); }); }); @@ -486,7 +477,7 @@ describe('MetricSelectRow', () => { await userEvent.click(metricSelector); await userEvent.click(await screen.findByRole('option', {name: 'counter_metric'})); - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ yAxis: serializeFields([ @@ -496,8 +487,7 @@ describe('MetricSelectRow', () => { }, ]), }), - }), - expect.anything() + }) ); }); }); diff --git a/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricSelectRow.tsx b/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricSelectRow.tsx index 2b1c298366e7..39769ebd4a0c 100644 --- a/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricSelectRow.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricSelectRow.tsx @@ -8,7 +8,10 @@ import { type QueryFieldValue, } from 'sentry/utils/discover/fields'; import {AggregateSelector} from 'sentry/views/dashboards/widgetBuilder/components/visualize/traceMetrics/aggregateSelector'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import { + useWidgetBuilderDispatch, + useWidgetBuilderStateSlice, +} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {useTraceMetricMultiMetricSelection} from 'sentry/views/dashboards/widgetBuilder/hooks/useTraceMetricMultiMetricSelection'; import { buildTraceMetricAggregate, @@ -66,7 +69,8 @@ export function MetricSelectRow({ field: QueryFieldValue; index: number; }) { - const {state, dispatch} = useWidgetBuilderContext(); + const state = useWidgetBuilderStateSlice('displayType', 'fields', 'yAxis'); + const dispatch = useWidgetBuilderDispatch(); const hasMultiMetricSelection = useTraceMetricMultiMetricSelection(); const aggregateSource = getTraceMetricAggregateSource( diff --git a/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricsEquationVisualize/builderStateMetricsQueryParamsProvider.tsx b/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricsEquationVisualize/builderStateMetricsQueryParamsProvider.tsx index c32e2c4b321e..92c0258555c1 100644 --- a/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricsEquationVisualize/builderStateMetricsQueryParamsProvider.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricsEquationVisualize/builderStateMetricsQueryParamsProvider.tsx @@ -2,7 +2,10 @@ import {useCallback} from 'react'; import {generateFieldAsString} from 'sentry/utils/discover/fields'; import {dispatchYAxisUpdate} from 'sentry/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricsEquationVisualize/utils'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import { + useWidgetBuilderDispatch, + useWidgetBuilderStateSlice, +} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {BuilderStateAction} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; import {getTraceMetricAggregateSource} from 'sentry/views/dashboards/widgetBuilder/utils/buildTraceMetricAggregate'; import { @@ -38,7 +41,8 @@ export function BuilderStateMetricsQueryParamsProvider({ onSubcomponentChanged, children, }: BuilderStateMetricsQueryParamsProviderProps) { - const {state, dispatch} = useWidgetBuilderContext(); + const state = useWidgetBuilderStateSlice('displayType', 'fields', 'yAxis'); + const dispatch = useWidgetBuilderDispatch(); const aggregateSource = getTraceMetricAggregateSource( state.displayType, diff --git a/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricsEquationVisualize/index.spec.tsx b/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricsEquationVisualize/index.spec.tsx index 0f6166b4f202..54097f09b372 100644 --- a/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricsEquationVisualize/index.spec.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricsEquationVisualize/index.spec.tsx @@ -8,15 +8,15 @@ import { within, } from 'sentry-test/reactTestingLibrary'; -import {useNavigate} from 'sentry/utils/useNavigate'; +import {replaceUrlWithoutNavigation} from 'sentry/utils/url/replaceUrlWithoutNavigation'; import {DisplayType, WidgetType} from 'sentry/views/dashboards/types'; import {MetricsEquationVisualize} from 'sentry/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricsEquationVisualize'; import {WidgetBuilderProvider} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {serializeFields} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; import {FieldValueKind} from 'sentry/views/discover/table/types'; -jest.mock('sentry/utils/useNavigate'); -const mockedUseNavigate = jest.mocked(useNavigate); +jest.mock('sentry/utils/url/replaceUrlWithoutNavigation'); +const mockReplaceUrl = jest.mocked(replaceUrlWithoutNavigation); const EQUATION_FEATURES = [ 'tracemetrics-enabled', @@ -59,11 +59,7 @@ function setupMockApis() { } describe('MetricsEquationVisualize', () => { - let mockNavigate!: jest.Mock; - beforeEach(() => { - mockNavigate = jest.fn(); - mockedUseNavigate.mockReturnValue(mockNavigate); setupMockApis(); }); @@ -100,7 +96,7 @@ describe('MetricsEquationVisualize', () => { await userEvent.click(radioButtons[1]!); await waitFor(() => { - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ yAxis: serializeFields([ @@ -110,8 +106,7 @@ describe('MetricsEquationVisualize', () => { }, ]), }), - }), - expect.anything() + }) ); }); }); diff --git a/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricsEquationVisualize/index.tsx b/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricsEquationVisualize/index.tsx index 7662509487ee..681f38f27ae3 100644 --- a/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricsEquationVisualize/index.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricsEquationVisualize/index.tsx @@ -4,7 +4,7 @@ import {defined} from 'sentry/utils/defined'; import {generateFieldAsString} from 'sentry/utils/discover/fields'; import {useOrganization} from 'sentry/utils/useOrganization'; import {MetricQueryRows} from 'sentry/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricsEquationVisualize/metricQueryRows'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import {useWidgetBuilderStateSlice} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import type {EquationModeSnapshot} from 'sentry/views/dashboards/widgetBuilder/hooks/useTraceMetricsVisualizeModeState'; import {getTraceMetricAggregateSource} from 'sentry/views/dashboards/widgetBuilder/utils/buildTraceMetricAggregate'; import {FieldValueKind} from 'sentry/views/discover/table/types'; @@ -34,7 +34,7 @@ export function MetricsEquationVisualize({ }: MetricsEquationVisualizeProps) { const organization = useOrganization(); const hasEquations = canUseMetricsEquationsInDashboards(organization); - const {state} = useWidgetBuilderContext(); + const state = useWidgetBuilderStateSlice('displayType', 'fields', 'query', 'yAxis'); const aggregateSource = getTraceMetricAggregateSource( state.displayType, diff --git a/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricsEquationVisualize/metricQueryRows.tsx b/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricsEquationVisualize/metricQueryRows.tsx index 2211c2730a7c..094e2c3bd7d5 100644 --- a/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricsEquationVisualize/metricQueryRows.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricsEquationVisualize/metricQueryRows.tsx @@ -11,7 +11,10 @@ import {generateFieldAsString} from 'sentry/utils/discover/fields'; import {BuilderStateMetricsQueryParamsProvider} from 'sentry/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricsEquationVisualize/builderStateMetricsQueryParamsProvider'; import {MetricToolbar} from 'sentry/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricsEquationVisualize/metricToolbar'; import {dispatchYAxisUpdate} from 'sentry/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricsEquationVisualize/utils'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import { + useWidgetBuilderDispatch, + useWidgetBuilderStateSlice, +} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import type {EquationModeSnapshot} from 'sentry/views/dashboards/widgetBuilder/hooks/useTraceMetricsVisualizeModeState'; import {BuilderStateAction} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; import {getTraceMetricAggregateSource} from 'sentry/views/dashboards/widgetBuilder/utils/buildTraceMetricAggregate'; @@ -60,7 +63,8 @@ export function MetricQueryRows({ setSelectedLabel, equationSnapshot, }: MetricQueryRowsProps) { - const {state, dispatch} = useWidgetBuilderContext(); + const state = useWidgetBuilderStateSlice('displayType', 'fields', 'yAxis'); + const dispatch = useWidgetBuilderDispatch(); const metricQueries = useMultiMetricsQueryParams(); // Keep the snapshot ref in sync so useTraceMetricsVisualizeModeState can diff --git a/static/app/views/dashboards/widgetBuilder/components/widgetBuilderSlideout.tsx b/static/app/views/dashboards/widgetBuilder/components/widgetBuilderSlideout.tsx index 0f8004e3384a..8ae5c003bae8 100644 --- a/static/app/views/dashboards/widgetBuilder/components/widgetBuilderSlideout.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/widgetBuilderSlideout.tsx @@ -61,7 +61,12 @@ import {WidgetBuilderTypeSelector} from 'sentry/views/dashboards/widgetBuilder/c import {Visualize} from 'sentry/views/dashboards/widgetBuilder/components/visualize'; import {WidgetTemplatesList} from 'sentry/views/dashboards/widgetBuilder/components/widgetTemplatesList'; import {WidgetBuilderXAxisSelector} from 'sentry/views/dashboards/widgetBuilder/components/xAxisSelector'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import { + useWidgetBuilderContext, + useWidgetBuilderDispatch, + useWidgetBuilderQueryState, + useWidgetBuilderStore, +} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {useCacheBuilderState} from 'sentry/views/dashboards/widgetBuilder/hooks/useCacheBuilderState'; import {useDashboardWidgetSource} from 'sentry/views/dashboards/widgetBuilder/hooks/useDashboardWidgetSource'; import {useDisableTransactionWidget} from 'sentry/views/dashboards/widgetBuilder/hooks/useDisableTransactionWidget'; @@ -106,33 +111,16 @@ function WidgetBuilderSlideoutInner({ const organization = useOrganization(); const location = useLocation(); const {visible: isModalVisible} = useModal(); - const {state, dispatch} = useWidgetBuilderContext(); - const [initialState] = useState(state); + // Subscribe to the query-affecting state only — title/description edits are + // high-frequency and should not re-render the whole slideout + const state = useWidgetBuilderQueryState(); + const dispatch = useWidgetBuilderDispatch(); + const store = useWidgetBuilderStore(); + const [initialState] = useState(() => store.getState()); const [customizeFromLibrary, setCustomizeFromLibrary] = useState(false); const [error, setError] = useState>({}); const theme = useTheme(); const isEditing = useIsEditingWidget(); - - // Push widget builder state into the LLM context tree for Seer Explorer. - useLLMContext({ - priority: 1, - contextHint: - 'Sentry widget builder. The user is configuring a dashboard widget. visualize is the y-axis metrics (timeseries) or the aggregate (big number/table). fields are group-by columns (timeseries) or visible columns (table). query filters the data and sort controls ordering.', - dashboardTitle: dashboard.title, - dashboardWidgetCount: dashboard.widgets.length, - dashboardFilters: dashboard.filters, - mode: isEditing ? 'editing' : 'creating', - title: state.title, - description: state.description, - dataset: state.dataset, - displayType: state.displayType, - visualize: state.yAxis?.map(generateFieldAsString), - fields: state.fields?.map(generateFieldAsString), - query: state.query?.map(readableConditions), - sort: state.sort?.map(s => (s.kind === 'desc' ? `-${s.field}` : s.field)), - thresholds: state.thresholds, - legendAlias: state.legendAlias, - }); const source = useDashboardWidgetSource(); const {cacheBuilderState} = useCacheBuilderState(); const {setSegmentSpanBuilderState} = useSegmentSpanWidgetState(); @@ -262,12 +250,12 @@ function WidgetBuilderSlideoutInner({ const onCloseWithModal = useCallback(() => { openConfirmModal({ - bypass: isEqual(initialState, state), + bypass: isEqual(initialState, store.getState()), message: t('You have unsaved changes. Are you sure you want to leave?'), priority: 'danger', onConfirm: onClose, }); - }, [initialState, onClose, state]); + }, [initialState, onClose, store]); useHotkeys([ { @@ -322,143 +310,88 @@ function WidgetBuilderSlideoutInner({ ); return ( - - {({isOpening}) => { - if (isOpening) { + + + + {({isOpening}) => { + if (isOpening) { + return ( + + {header} + + + + + + + + ); + } + return ( {header} - - - - - - - - ); - } - - return ( - - {header} - - {isTransactionsWidget && showTransactionsDeprecationAlert && ( -
- } - aria-label={t('Close')} - onClick={() => { - setShowTransactionsDeprecationAlert(false); - }} - size="zero" - variant="transparent" - /> - } - > - {disableTransactionWidget && isEditing - ? tctCode( - 'Editing of transaction-based widgets is disabled, as we migrate to the span dataset. To expedite and re-enable edit functionality, switch to the [spans] dataset below with the [code:is_transaction:true] filter. Please read these [FAQLink:FAQs] for more information.', - { - spans: ( - { - cacheBuilderState(state.dataset ?? WidgetType.ERRORS); - setSegmentSpanBuilderState(); - }} - > - {t('spans')} - - ), - FAQLink: ( - - ), - } - ) - : tctCode( - 'The transactions dataset is being deprecated. Please use the Spans dataset with the [code:is_transaction:true] filter instead. Please read these [FAQLink:FAQs] for more information.', - { - FAQLink: ( - - ), - } - )} - -
- )} - {openWidgetTemplates ? ( - -
- {isSmallScreen && ( -
- + {isTransactionsWidget && showTransactionsDeprecationAlert && ( +
+ } + aria-label={t('Close')} + onClick={() => { + setShowTransactionsDeprecationAlert(false); + }} + size="zero" + variant="transparent" /> -
- )} -
- {isSmallScreen && ( -
- -
- )} - - - ) : ( - - -
- -
- - {showDatasetSelector && ( -
- -
- )} - -
- - {isTimeSeriesWidget && } - {isTimeSeriesWidget && } -
- {isTextWidget && ( -
- -
- )} + } + > + {disableTransactionWidget && isEditing + ? tctCode( + 'Editing of transaction-based widgets is disabled, as we migrate to the span dataset. To expedite and re-enable edit functionality, switch to the [spans] dataset below with the [code:is_transaction:true] filter. Please read these [FAQLink:FAQs] for more information.', + { + spans: ( + { + cacheBuilderState(state.dataset ?? WidgetType.ERRORS); + setSegmentSpanBuilderState(); + }} + > + {t('spans')} + + ), + FAQLink: ( + + ), + } + ) + : tctCode( + 'The transactions dataset is being deprecated. Please use the Spans dataset with the [code:is_transaction:true] filter instead. Please read these [FAQLink:FAQs] for more information.', + { + FAQLink: ( + + ), + } + )} + + + )} + {openWidgetTemplates ? ( +
{isSmallScreen && (
@@ -479,67 +412,162 @@ function WidgetBuilderSlideoutInner({ />
)} - {showXAxisSelector && ( -
- -
- )} - {showVisualizeSection && ( -
- -
- )} - {showQueryFilterBuilder && ( + + + ) : ( + +
- -
- )} - {doesDisplayTypeSupportThresholds(state.displayType) && ( -
-
- )} - {showGroupBySelector && ( + + {showDatasetSelector && (
- +
)} - {showSortByStep && ( +
- + + {isTimeSeriesWidget && } + {isTimeSeriesWidget && }
- )} - - - - )} - - - ); - }} - + {isTextWidget && ( +
+ +
+ )} +
+ {isSmallScreen && ( +
+ +
+ )} +
+ {isSmallScreen && ( +
+ +
+ )} + {showXAxisSelector && ( +
+ +
+ )} + {showVisualizeSection && ( +
+ +
+ )} + {showQueryFilterBuilder && ( +
+ +
+ )} + {doesDisplayTypeSupportThresholds(state.displayType) && ( +
+ +
+ )} + {showGroupBySelector && ( +
+ +
+ )} + {showSortByStep && ( +
+ +
+ )} + + + + )} + + + ); + }} + + ); } +/** + * Pushes the widget builder state into the LLM context tree for Seer + * Explorer. Rendered as a null component so its full-state subscription + * doesn't re-render the slideout on every keystroke. + */ +function SlideoutLLMContext({ + dashboard, + isEditing, +}: { + dashboard: DashboardDetails; + isEditing: boolean; +}) { + const {state} = useWidgetBuilderContext(); + + useLLMContext({ + priority: 1, + contextHint: + 'Sentry widget builder. The user is configuring a dashboard widget. visualize is the y-axis metrics (timeseries) or the aggregate (big number/table). fields are group-by columns (timeseries) or visible columns (table). query filters the data and sort controls ordering.', + dashboardTitle: dashboard.title, + dashboardWidgetCount: dashboard.widgets.length, + dashboardFilters: dashboard.filters, + mode: isEditing ? 'editing' : 'creating', + title: state.title, + description: state.description, + dataset: state.dataset, + displayType: state.displayType, + visualize: state.yAxis?.map(generateFieldAsString), + fields: state.fields?.map(generateFieldAsString), + query: state.query?.map(readableConditions), + sort: state.sort?.map(s => (s.kind === 'desc' ? `-${s.field}` : s.field)), + thresholds: state.thresholds, + legendAlias: state.legendAlias, + }); + + return null; +} + export const WidgetBuilderSlideout = registerLLMContext( 'widget-builder', WidgetBuilderSlideoutInner diff --git a/static/app/views/dashboards/widgetBuilder/components/widgetTemplatesList.spec.tsx b/static/app/views/dashboards/widgetBuilder/components/widgetTemplatesList.spec.tsx index 597ab3282b0a..3e8b23e55b50 100644 --- a/static/app/views/dashboards/widgetBuilder/components/widgetTemplatesList.spec.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/widgetTemplatesList.spec.tsx @@ -3,16 +3,14 @@ import debounce from 'lodash/debounce'; import {render, screen, userEvent, waitFor} from 'sentry-test/reactTestingLibrary'; import {addErrorMessage} from 'sentry/actionCreators/indicator'; +import {replaceUrlWithoutNavigation} from 'sentry/utils/url/replaceUrlWithoutNavigation'; import {testableDebounce} from 'sentry/utils/url/testUtils'; -import {useNavigate} from 'sentry/utils/useNavigate'; import {WidgetTemplatesList} from 'sentry/views/dashboards/widgetBuilder/components/widgetTemplatesList'; import {WidgetBuilderProvider} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {getDefaultWidgets} from 'sentry/views/dashboards/widgetLibrary/data'; import type {WidgetTemplate} from 'sentry/views/dashboards/widgetLibrary/types'; -jest.mock('sentry/utils/useNavigate', () => ({ - useNavigate: jest.fn(), -})); +jest.mock('sentry/utils/url/replaceUrlWithoutNavigation'); jest.mock('lodash/debounce'); jest.mock('sentry/views/dashboards/widgetLibrary/data', () => ({ @@ -29,7 +27,7 @@ jest.mock('sentry/views/dashboards/widgetLibrary/data', () => ({ ]), })); -const mockUseNavigate = jest.mocked(useNavigate); +const mockReplaceUrl = jest.mocked(replaceUrlWithoutNavigation); const mockGetDefaultWidgets = jest.mocked(getDefaultWidgets); jest.mock('sentry/actionCreators/indicator'); @@ -40,9 +38,6 @@ describe('WidgetTemplatesList', () => { beforeEach(() => { jest.useFakeTimers(); - const mockNavigate = jest.fn(); - mockUseNavigate.mockReturnValue(mockNavigate); - jest.mocked(debounce).mockImplementation(testableDebounce); MockApiClient.addMockResponse({ @@ -101,9 +96,6 @@ describe('WidgetTemplatesList', () => { it('should put widget in url when clicking a template', async () => { const user = userEvent.setup({advanceTimers: jest.advanceTimersByTime}); - const mockNavigate = jest.fn(); - mockUseNavigate.mockReturnValue(mockNavigate); - render( { jest.runAllTimers(); - expect(mockNavigate).toHaveBeenLastCalledWith( + expect(mockReplaceUrl).toHaveBeenLastCalledWith( expect.objectContaining({ query: expect.objectContaining({ description: 'some description', @@ -128,8 +120,7 @@ describe('WidgetTemplatesList', () => { displayType: 'line', dataset: 'transactions-like', }), - }), - expect.anything() + }) ); }); diff --git a/static/app/views/dashboards/widgetBuilder/components/widgetTemplatesList.tsx b/static/app/views/dashboards/widgetBuilder/components/widgetTemplatesList.tsx index fd115d5570cb..92e6c33e9d90 100644 --- a/static/app/views/dashboards/widgetBuilder/components/widgetTemplatesList.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/widgetTemplatesList.tsx @@ -15,7 +15,7 @@ import {useApi} from 'sentry/utils/useApi'; import {useLocation} from 'sentry/utils/useLocation'; import {useOrganization} from 'sentry/utils/useOrganization'; import type {Widget} from 'sentry/views/dashboards/types'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import {useWidgetBuilderDispatch} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {BuilderStateAction} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; import {convertWidgetToBuilderState} from 'sentry/views/dashboards/widgetBuilder/utils/convertWidgetToBuilderStateParams'; import {getDefaultWidgets} from 'sentry/views/dashboards/widgetLibrary/data'; @@ -48,7 +48,7 @@ export function WidgetTemplatesList({ : null; const [selectedWidget, setSelectedWidget] = useState(initialSelectedIndex); - const {dispatch} = useWidgetBuilderContext(); + const dispatch = useWidgetBuilderDispatch(); const {widgetIndex} = useParams(); const api = useApi(); diff --git a/static/app/views/dashboards/widgetBuilder/components/xAxisSelector.tsx b/static/app/views/dashboards/widgetBuilder/components/xAxisSelector.tsx index 5a53c09fa441..73c132f8b6ba 100644 --- a/static/app/views/dashboards/widgetBuilder/components/xAxisSelector.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/xAxisSelector.tsx @@ -12,7 +12,10 @@ import {useTags} from 'sentry/utils/useTags'; import {getDatasetConfig} from 'sentry/views/dashboards/datasetConfig/base'; import {WidgetType} from 'sentry/views/dashboards/types'; import {SectionHeader} from 'sentry/views/dashboards/widgetBuilder/components/common/sectionHeader'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import { + useWidgetBuilderDispatch, + useWidgetBuilderStateSlice, +} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {useDashboardWidgetSource} from 'sentry/views/dashboards/widgetBuilder/hooks/useDashboardWidgetSource'; import {useIsEditingWidget} from 'sentry/views/dashboards/widgetBuilder/hooks/useIsEditingWidget'; import {BuilderStateAction} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; @@ -26,7 +29,8 @@ import {HiddenTraceMetricGroupByFields} from 'sentry/views/explore/metrics/const export function WidgetBuilderXAxisSelector() { const organization = useOrganization(); - const {state, dispatch} = useWidgetBuilderContext(); + const state = useWidgetBuilderStateSlice('dataset', 'fields'); + const dispatch = useWidgetBuilderDispatch(); const source = useDashboardWidgetSource(); const isEditing = useIsEditingWidget(); diff --git a/static/app/views/dashboards/widgetBuilder/contexts/widgetBuilderContext.spec.tsx b/static/app/views/dashboards/widgetBuilder/contexts/widgetBuilderContext.spec.tsx new file mode 100644 index 000000000000..147cdb684bdc --- /dev/null +++ b/static/app/views/dashboards/widgetBuilder/contexts/widgetBuilderContext.spec.tsx @@ -0,0 +1,97 @@ +import {Fragment} from 'react'; + +import {act, render, screen} from 'sentry-test/reactTestingLibrary'; + +import { + useWidgetBuilderDispatch, + useWidgetBuilderStateSlice, + WidgetBuilderProvider, +} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import {BuilderStateAction} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; + +describe('WidgetBuilderProvider selectors', () => { + let titleRenders: number; + let queryRenders: number; + let capturedDispatch: ReturnType; + + function TitleConsumer() { + titleRenders++; + const {title} = useWidgetBuilderStateSlice('title'); + return
title:{title}
; + } + + function QueryConsumer() { + queryRenders++; + const {query} = useWidgetBuilderStateSlice('query'); + return
query:{query?.join(',')}
; + } + + function DispatchGrabber() { + capturedDispatch = useWidgetBuilderDispatch(); + return null; + } + + beforeEach(() => { + titleRenders = 0; + queryRenders = 0; + }); + + it('only re-renders consumers subscribed to the changed field', () => { + render( + + + + + + + + ); + + const titleRendersBefore = titleRenders; + const queryRendersBefore = queryRenders; + + act(() => { + capturedDispatch({ + type: BuilderStateAction.SET_TITLE, + payload: 'a new title', + }); + }); + + expect(screen.getByText('title:a new title')).toBeInTheDocument(); + expect(titleRenders).toBeGreaterThan(titleRendersBefore); + // The query consumer must not re-render for a title change + expect(queryRenders).toBe(queryRendersBefore); + }); + + it('persists state to the URL without notifying the router', () => { + jest.useFakeTimers(); + window.history.replaceState(null, '', '/mock-pathname/'); + + const {router} = render( + + + , + { + initialRouterConfig: { + location: {pathname: '/mock-pathname/'}, + }, + } + ); + + act(() => { + capturedDispatch({ + type: BuilderStateAction.SET_TITLE, + payload: 'url title', + }); + jest.runAllTimers(); + }); + + // The browser URL has the new state for refreshes and copied links + expect(window.location.search).toContain('title=url%20title'); + // ...but the router (and all of its location subscribers) was not updated + expect(router.location.query).toEqual({}); + + jest.useRealTimers(); + window.history.replaceState(null, '', '/'); + }); +}); diff --git a/static/app/views/dashboards/widgetBuilder/contexts/widgetBuilderContext.tsx b/static/app/views/dashboards/widgetBuilder/contexts/widgetBuilderContext.tsx index 4ad063e4d550..9323a0ce9912 100644 --- a/static/app/views/dashboards/widgetBuilder/contexts/widgetBuilderContext.tsx +++ b/static/app/views/dashboards/widgetBuilder/contexts/widgetBuilderContext.tsx @@ -1,39 +1,129 @@ import type React from 'react'; -import {createContext, useContext} from 'react'; +import { + createContext, + useCallback, + useContext, + useEffect, + useRef, + useState, + useSyncExternalStore, +} from 'react'; -import {UrlParamBatchProvider} from 'sentry/utils/url/urlParamBatchContext'; -import {useWidgetBuilderState} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; +import {useLocation} from 'sentry/utils/useLocation'; +import { + useWidgetBuilderState, + type WidgetBuilderState, + type WidgetBuilderStore, +} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; -const WidgetBuilderContext = createContext< - ReturnType | undefined ->(undefined); +type WidgetBuilderContextValue = Pick< + ReturnType, + 'state' | 'dispatch' +>; + +const WidgetBuilderContext = createContext( + undefined +); + +/** + * Carries the widget builder store itself. The store's identity is stable for + * the lifetime of the builder, so consuming this context never causes a + * re-render — subscriptions happen through the selector hooks below. + */ +const WidgetBuilderStoreContext = createContext( + undefined +); interface WidgetBuilderProviderProps { children: React.ReactNode; } +const EMPTY_STATE: WidgetBuilderState = {}; +const EMPTY_URL_PARAMS = {}; + +/** + * Builds a read-only store view over a mocked `useWidgetBuilderState` return + * value so the selector hooks keep working in tests that mock the hook (e.g. + * dashboard detail tests). The mocked state is static, so subscriptions are + * no-ops; values are read through the ref so consumers see the latest mock. + */ +function createFallbackStore( + latestRef: React.RefObject +): WidgetBuilderStore { + return { + dispatch: (action, options) => latestRef.current?.dispatch(action, options), + getState: () => latestRef.current?.state ?? EMPTY_STATE, + getUrlParams: () => EMPTY_URL_PARAMS, + subscribe: () => () => {}, + syncUrlParams: () => {}, + teardown: () => {}, + }; +} + /** * Provider for maintaining a single source of truth for the widget builder state. */ function WidgetBuilderStateProvider({children}: WidgetBuilderProviderProps) { const widgetBuilderState = useWidgetBuilderState(); + + // `useWidgetBuilderState` may be jest-mocked without a `store`; fall back to + // a static store view over the mocked state so selector hooks keep working. + const latestRef = useRef(widgetBuilderState); + latestRef.current = widgetBuilderState; + const [fallbackStore] = useState(() => + widgetBuilderState?.store ? null : createFallbackStore(latestRef) + ); + return ( - - {children} - + + + + {children} + + ); } export function WidgetBuilderProvider({children}: WidgetBuilderProviderProps) { - return ( - - {children} - - ); + return {children}; +} + +/** + * The widget builder store writes its query params to the URL without + * notifying the router. When something else navigates (e.g. a page filter + * change), the router rebuilds the URL from its own location and drops those + * params, so re-assert them here. Renders nothing, so the `useLocation` + * subscription is cheap. + */ +function WidgetBuilderUrlParamSync() { + const store = useContext(WidgetBuilderStoreContext); + const location = useLocation(); + const isFirstRender = useRef(true); + + useEffect(() => { + if (isFirstRender.current) { + isFirstRender.current = false; + return; + } + + // Skip when navigating away from the builder (e.g. closing it), where the + // params are being cleaned up intentionally + if (location.pathname.includes('/widget-builder/')) { + store?.syncUrlParams(); + } + }, [location, store]); + + return null; } /** * Custom hook to get state and dispatch from the WidgetBuilderContext + * + * Subscribes to the full widget builder state — any state change re-renders + * the caller. Prefer `useWidgetBuilderStateSlice`/`useWidgetBuilderDispatch` + * unless the whole state is genuinely needed at render time. */ export const useWidgetBuilderContext = () => { const context = useContext(WidgetBuilderContext); @@ -44,3 +134,110 @@ export const useWidgetBuilderContext = () => { } return context; }; + +/** + * Returns the widget builder store. Useful for reading state inside event + * handlers (`store.getState()`) without subscribing to re-renders. + */ +export function useWidgetBuilderStore(): WidgetBuilderStore { + const store = useContext(WidgetBuilderStoreContext); + if (!store) { + throw new Error('useWidgetBuilderStore must be used within a WidgetBuilderProvider'); + } + return store; +} + +/** + * Returns the widget builder dispatch function without subscribing to any + * state changes. + */ +export function useWidgetBuilderDispatch(): WidgetBuilderStore['dispatch'] { + return useWidgetBuilderStore().dispatch; +} + +/** + * Subscribes to a subset of the widget builder state. The caller only + * re-renders when one of the selected fields changes. + * + * const {dataset, displayType} = useWidgetBuilderStateSlice('dataset', 'displayType'); + */ +export function useWidgetBuilderStateSlice( + ...keys: K[] +): Pick { + const store = useWidgetBuilderStore(); + + const keysRef = useRef(keys); + keysRef.current = keys; + const cacheRef = useRef<{ + selection: Pick; + snapshot: WidgetBuilderState; + } | null>(null); + + const getSnapshot = useCallback(() => { + const snapshot = store.getState(); + const cached = cacheRef.current; + if (cached?.snapshot === snapshot) { + return cached.selection; + } + + const selection = {} as Pick; + for (const key of keysRef.current) { + selection[key] = snapshot[key]; + } + + // Keep the previous selection's identity when the selected fields are + // unchanged so subscribers bail out of re-rendering + if ( + cached && + keysRef.current.every(key => Object.is(cached.selection[key], selection[key])) + ) { + cacheRef.current = {snapshot, selection: cached.selection}; + return cached.selection; + } + + cacheRef.current = {snapshot, selection}; + return selection; + }, [store]); + + return useSyncExternalStore(store.subscribe, getSnapshot); +} + +const WIDGET_QUERY_STATE_KEYS = [ + 'axisRange', + 'dataset', + 'displayType', + 'fields', + 'legendAlias', + 'legendType', + 'limit', + 'linkedDashboards', + 'query', + 'selectedAggregate', + 'sort', + 'thresholds', + 'yAxis', +] as const; + +/** + * Subscribes to every widget builder state field that affects the widget's + * queries — i.e. everything except `title`, `description`, and `textContent`. + * Use for components that derive the widget query at render time (e.g. via + * `convertBuilderStateToWidget`) but should not re-render while the user + * types a title or description. + */ +export function useWidgetBuilderQueryState(): Pick< + WidgetBuilderState, + (typeof WIDGET_QUERY_STATE_KEYS)[number] +> { + return useWidgetBuilderStateSlice(...WIDGET_QUERY_STATE_KEYS); +} + +/** + * Subscribes to the serialized query params the widget builder has written to + * the URL. Merge these over `location.query` when checking the URL for widget + * state, since the store writes the URL without notifying the router. + */ +export function useWidgetBuilderUrlParams() { + const store = useWidgetBuilderStore(); + return useSyncExternalStore(store.subscribe, store.getUrlParams); +} diff --git a/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.spec.tsx b/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.spec.tsx index 43a0bb07b73b..81f10c550b21 100644 --- a/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.spec.tsx +++ b/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.spec.tsx @@ -5,32 +5,44 @@ import {renderHook} from 'sentry-test/reactTestingLibrary'; import {useLocation} from 'sentry/utils/useLocation'; import {DisplayType, WidgetType} from 'sentry/views/dashboards/types'; import { - useWidgetBuilderContext, + useWidgetBuilderStore, WidgetBuilderProvider, } from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; -import { - BuilderStateAction, - type WidgetBuilderState, +import type { + WidgetBuilderState, + WidgetBuilderStore, } from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; +import {BuilderStateAction} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; import {convertBuilderStateToWidget} from 'sentry/views/dashboards/widgetBuilder/utils/convertBuilderStateToWidget'; import {useCacheBuilderState} from './useCacheBuilderState'; -jest.mock('sentry/utils/useNavigate', () => ({ - useNavigate: jest.fn(), -})); jest.mock('sentry/utils/useLocation'); jest.mock('sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext', () => ({ - useWidgetBuilderContext: jest.fn(), + useWidgetBuilderStore: jest.fn(), WidgetBuilderProvider: jest.requireActual( 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext' ).WidgetBuilderProvider, })); -const mockUseWidgetBuilderContext = jest.mocked(useWidgetBuilderContext); +const mockUseWidgetBuilderStore = jest.mocked(useWidgetBuilderStore); const mockUseLocation = jest.mocked(useLocation); +function mockStore( + state: WidgetBuilderState, + dispatch: WidgetBuilderStore['dispatch'] = jest.fn() +) { + mockUseWidgetBuilderStore.mockReturnValue({ + dispatch, + getState: () => state, + getUrlParams: () => ({}), + subscribe: () => () => {}, + syncUrlParams: () => {}, + teardown: () => {}, + }); +} + function Wrapper({children}: {children: React.ReactNode}) { return {children}; } @@ -40,10 +52,7 @@ describe('useCacheBuilderState', () => { beforeEach(() => { mockLocalStorage = {}; - mockUseWidgetBuilderContext.mockReturnValue({ - state: {}, - dispatch: jest.fn(), - }); + mockStore({}); mockUseLocation.mockReturnValue(LocationFixture()); Storage.prototype.getItem = jest.fn(key => mockLocalStorage[key] ?? null); @@ -71,10 +80,7 @@ describe('useCacheBuilderState', () => { ], query: ['this is a test query'], }; - mockUseWidgetBuilderContext.mockReturnValue({ - state: cachedWidget, - dispatch: jest.fn(), - }); + mockStore(cachedWidget); const {result} = renderHook(() => useCacheBuilderState(), { wrapper: Wrapper, @@ -123,10 +129,7 @@ describe('useCacheBuilderState', () => { ], }; const mockDispatch = jest.fn(); - mockUseWidgetBuilderContext.mockReturnValue({ - state: currentWidget, - dispatch: mockDispatch, - }); + mockStore(currentWidget, mockDispatch); // Add cached widget to the localStorage localStorage.setItem( 'dashboards:widget-builder:dataset:error-events', @@ -177,10 +180,7 @@ describe('useCacheBuilderState', () => { ], }; const mockDispatch = jest.fn(); - mockUseWidgetBuilderContext.mockReturnValue({ - state: currentWidget, - dispatch: mockDispatch, - }); + mockStore(currentWidget, mockDispatch); // Add cached widget to the localStorage, this will not be the one // used in the test to test that a cache miss falls back to the plain // dataset change diff --git a/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.tsx b/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.tsx index 0ede6134af1d..1d4ec1f74fe9 100644 --- a/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.tsx +++ b/static/app/views/dashboards/widgetBuilder/hooks/useCacheBuilderState.tsx @@ -2,7 +2,7 @@ import {useCallback, useEffect} from 'react'; import {createStorage} from 'sentry/utils/createStorage'; import type {WidgetType} from 'sentry/views/dashboards/types'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import {useWidgetBuilderStore} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {BuilderStateAction} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; import {convertBuilderStateToWidget} from 'sentry/views/dashboards/widgetBuilder/utils/convertBuilderStateToWidget'; import {convertWidgetToBuilderState} from 'sentry/views/dashboards/widgetBuilder/utils/convertWidgetToBuilderStateParams'; @@ -25,7 +25,10 @@ function cleanUpDatasetState() { * and restore it when the user navigates back to the same dataset. */ export function useCacheBuilderState() { - const {state, dispatch} = useWidgetBuilderContext(); + // The state is only read inside the returned callbacks, so use the store + // directly instead of subscribing to every state change + const store = useWidgetBuilderStore(); + const dispatch = store.dispatch; useEffect(() => { // Remove all cached dataset states when the component mounts @@ -39,10 +42,10 @@ export function useCacheBuilderState() { (dataset: WidgetType) => { STORAGE.setItem( `${WIDGET_BUILDER_DATASET_STATE_KEY}:${dataset}`, - JSON.stringify(convertBuilderStateToWidget(state)) + JSON.stringify(convertBuilderStateToWidget(store.getState())) ); }, - [state] + [store] ); // Checks if there is a cached builder state for the given dataset @@ -56,9 +59,10 @@ export function useCacheBuilderState() { const builderState = convertWidgetToBuilderState( JSON.parse(previousDatasetState) ); + const {title, description} = store.getState(); dispatch({ type: BuilderStateAction.SET_STATE, - payload: {...builderState, title: state.title, description: state.description}, + payload: {...builderState, title, description}, }); } else { dispatch({ @@ -67,7 +71,7 @@ export function useCacheBuilderState() { }); } }, - [dispatch, state.title, state.description] + [dispatch, store] ); return { diff --git a/static/app/views/dashboards/widgetBuilder/hooks/useDisableTransactionWidget.tsx b/static/app/views/dashboards/widgetBuilder/hooks/useDisableTransactionWidget.tsx index 2955863fa8c4..15e2295847f6 100644 --- a/static/app/views/dashboards/widgetBuilder/hooks/useDisableTransactionWidget.tsx +++ b/static/app/views/dashboards/widgetBuilder/hooks/useDisableTransactionWidget.tsx @@ -1,9 +1,9 @@ import {useOrganization} from 'sentry/utils/useOrganization'; import {WidgetType} from 'sentry/views/dashboards/types'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import {useWidgetBuilderStateSlice} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; export function useDisableTransactionWidget() { - const {state} = useWidgetBuilderContext(); + const state = useWidgetBuilderStateSlice('dataset'); const organization = useOrganization(); return ( diff --git a/static/app/views/dashboards/widgetBuilder/hooks/useSegmentSpanWidgetState.tsx b/static/app/views/dashboards/widgetBuilder/hooks/useSegmentSpanWidgetState.tsx index bb94304f2381..8fec2594eca7 100644 --- a/static/app/views/dashboards/widgetBuilder/hooks/useSegmentSpanWidgetState.tsx +++ b/static/app/views/dashboards/widgetBuilder/hooks/useSegmentSpanWidgetState.tsx @@ -1,17 +1,19 @@ import {useCallback} from 'react'; import {WidgetType} from 'sentry/views/dashboards/types'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import {useWidgetBuilderStore} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {BuilderStateAction} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; import {convertBuilderStateToStateQueryParams} from 'sentry/views/dashboards/widgetBuilder/utils/convertBuilderStateToStateQueryParams'; export function useSegmentSpanWidgetState() { - const {dispatch, state} = useWidgetBuilderContext(); + // The state is only read inside the returned callback, so use the store + // directly instead of subscribing to every state change + const store = useWidgetBuilderStore(); const setSegmentSpanBuilderState = useCallback(() => { const nextDataset = WidgetType.SPANS; - const stateParams = convertBuilderStateToStateQueryParams(state); - dispatch({ + const stateParams = convertBuilderStateToStateQueryParams(store.getState()); + store.dispatch({ type: BuilderStateAction.SET_STATE, payload: { ...stateParams, @@ -19,7 +21,7 @@ export function useSegmentSpanWidgetState() { query: ['is_transaction:true'], }, }); - }, [dispatch, state]); + }, [store]); return { setSegmentSpanBuilderState, diff --git a/static/app/views/dashboards/widgetBuilder/hooks/useTraceMetricsVisualizeModeState.spec.tsx b/static/app/views/dashboards/widgetBuilder/hooks/useTraceMetricsVisualizeModeState.spec.tsx index 68e5fa435355..5c0faafdfd40 100644 --- a/static/app/views/dashboards/widgetBuilder/hooks/useTraceMetricsVisualizeModeState.spec.tsx +++ b/static/app/views/dashboards/widgetBuilder/hooks/useTraceMetricsVisualizeModeState.spec.tsx @@ -2,7 +2,7 @@ import {OrganizationFixture} from 'sentry-fixture/organization'; import {act, renderHookWithProviders, waitFor} from 'sentry-test/reactTestingLibrary'; -import {useNavigate} from 'sentry/utils/useNavigate'; +import {replaceUrlWithoutNavigation} from 'sentry/utils/url/replaceUrlWithoutNavigation'; import {DisplayType, WidgetType} from 'sentry/views/dashboards/types'; import { useWidgetBuilderContext, @@ -21,8 +21,8 @@ import { VisualizeFunction, } from 'sentry/views/explore/queryParams/visualize'; -jest.mock('sentry/utils/useNavigate'); -const mockedUseNavigate = jest.mocked(useNavigate); +jest.mock('sentry/utils/url/replaceUrlWithoutNavigation'); +const mockReplaceUrl = jest.mocked(replaceUrlWithoutNavigation); const EQUATION_FEATURES = [ 'tracemetrics-enabled', @@ -93,12 +93,7 @@ function makeEquationSnapshot( } describe('useTraceMetricsVisualizeModeState', () => { - let mockNavigate!: jest.Mock; - - beforeEach(() => { - mockNavigate = jest.fn(); - mockedUseNavigate.mockReturnValue(mockNavigate); - }); + beforeEach(() => {}); afterEach(() => { jest.clearAllMocks(); @@ -235,13 +230,13 @@ describe('useTraceMetricsVisualizeModeState', () => { result.current.handleModeToggle(true); }); - mockNavigate.mockClear(); + mockReplaceUrl.mockClear(); act(() => { result.current.handleModeToggle(false); }); await waitFor(() => { - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ yAxis: serializeFields([ @@ -251,19 +246,17 @@ describe('useTraceMetricsVisualizeModeState', () => { }, ]), }), - }), - expect.anything() + }) ); }); await waitFor(() => { - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ query: ['environment:prod'], }), - }), - expect.anything() + }) ); }); }); @@ -332,13 +325,13 @@ describe('useTraceMetricsVisualizeModeState', () => { }); }); - mockNavigate.mockClear(); + mockReplaceUrl.mockClear(); act(() => { result.current.handleModeToggle(true); }); await waitFor(() => { - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ yAxis: serializeFields([ @@ -348,8 +341,7 @@ describe('useTraceMetricsVisualizeModeState', () => { }, ]), }), - }), - expect.anything() + }) ); }); }); @@ -376,19 +368,18 @@ describe('useTraceMetricsVisualizeModeState', () => { }); }); - mockNavigate.mockClear(); + mockReplaceUrl.mockClear(); act(() => { result.current.handleModeToggle(true); }); await waitFor(() => { - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ query: ['environment:prod'], }), - }), - expect.anything() + }) ); }); }); @@ -418,19 +409,18 @@ describe('useTraceMetricsVisualizeModeState', () => { }); }); - mockNavigate.mockClear(); + mockReplaceUrl.mockClear(); act(() => { result.current.handleModeToggle(true); }); await waitFor(() => { - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ yAxis: ['sum(value,alpha_metric,counter,none)'], }), - }), - expect.anything() + }) ); }); }); @@ -459,13 +449,13 @@ describe('useTraceMetricsVisualizeModeState', () => { result.current.equationSnapshot.current = makeEquationSnapshot(); }); - mockNavigate.mockClear(); + mockReplaceUrl.mockClear(); act(() => { result.current.handleModeToggle(false); }); await waitFor(() => { - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ yAxis: serializeFields([ @@ -479,8 +469,7 @@ describe('useTraceMetricsVisualizeModeState', () => { }, ]), }), - }), - expect.anything() + }) ); }); }); @@ -509,13 +498,13 @@ describe('useTraceMetricsVisualizeModeState', () => { }); }); - mockNavigate.mockClear(); + mockReplaceUrl.mockClear(); act(() => { result.current.handleModeToggle(false); }); await waitFor(() => { - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ yAxis: serializeFields([ @@ -529,8 +518,7 @@ describe('useTraceMetricsVisualizeModeState', () => { }, ]), }), - }), - expect.anything() + }) ); }); }); @@ -555,13 +543,13 @@ describe('useTraceMetricsVisualizeModeState', () => { expect(result.current.isEquationMode).toBe(true); - mockNavigate.mockClear(); + mockReplaceUrl.mockClear(); act(() => { result.current.handleModeToggle(false); }); await waitFor(() => { - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ yAxis: serializeFields([ @@ -571,8 +559,7 @@ describe('useTraceMetricsVisualizeModeState', () => { }, ]), }), - }), - expect.anything() + }) ); }); }); @@ -593,7 +580,7 @@ describe('useTraceMetricsVisualizeModeState', () => { }, }); - mockNavigate.mockClear(); + mockReplaceUrl.mockClear(); act(() => { result.current.handleModeToggle(true); }); @@ -603,13 +590,12 @@ describe('useTraceMetricsVisualizeModeState', () => { expect(result.current.equationSnapshot.current?.selectedLabel).toBe('ƒ1'); await waitFor(() => { - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ yAxis: ['equation|'], }), - }), - expect.anything() + }) ); }); }); diff --git a/static/app/views/dashboards/widgetBuilder/hooks/useTraceMetricsVisualizeModeState.tsx b/static/app/views/dashboards/widgetBuilder/hooks/useTraceMetricsVisualizeModeState.tsx index 76c80da83df8..47f3cffaf909 100644 --- a/static/app/views/dashboards/widgetBuilder/hooks/useTraceMetricsVisualizeModeState.tsx +++ b/static/app/views/dashboards/widgetBuilder/hooks/useTraceMetricsVisualizeModeState.tsx @@ -9,7 +9,10 @@ import {useOrganization} from 'sentry/utils/useOrganization'; import {getDatasetConfig} from 'sentry/views/dashboards/datasetConfig/base'; import {WidgetType} from 'sentry/views/dashboards/types'; import {dispatchYAxisUpdate} from 'sentry/views/dashboards/widgetBuilder/components/visualize/traceMetrics/metricsEquationVisualize/utils'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import { + useWidgetBuilderDispatch, + useWidgetBuilderStateSlice, +} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {BuilderStateAction} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState'; import { getTraceMetricAggregateActionType, @@ -56,7 +59,15 @@ export interface TraceMetricsVisualizeModeState { */ export function useTraceMetricsVisualizeModeState(): TraceMetricsVisualizeModeState { const organization = useOrganization(); - const {state, dispatch} = useWidgetBuilderContext(); + const state = useWidgetBuilderStateSlice( + 'dataset', + 'displayType', + 'fields', + 'legendAlias', + 'query', + 'yAxis' + ); + const dispatch = useWidgetBuilderDispatch(); const hasEquations = canUseMetricsEquationsInDashboards(organization); diff --git a/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.spec.tsx b/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.spec.tsx index bf84c1d7e24f..e07d45270c2e 100644 --- a/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.spec.tsx +++ b/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.spec.tsx @@ -3,8 +3,8 @@ import {LocationFixture} from 'sentry-fixture/locationFixture'; import {act, renderHook} from 'sentry-test/reactTestingLibrary'; import type {AggregationKeyWithAlias, Column} from 'sentry/utils/discover/fields'; +import {replaceUrlWithoutNavigation} from 'sentry/utils/url/replaceUrlWithoutNavigation'; import {useLocation} from 'sentry/utils/useLocation'; -import {useNavigate} from 'sentry/utils/useNavigate'; import {DisplayType, WidgetType} from 'sentry/views/dashboards/types'; import {WidgetBuilderProvider} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import { @@ -15,16 +15,13 @@ import { import {FieldValueKind} from 'sentry/views/discover/table/types'; jest.mock('sentry/utils/useLocation'); -jest.mock('sentry/utils/useNavigate'); +jest.mock('sentry/utils/url/replaceUrlWithoutNavigation'); const mockedUsedLocation = jest.mocked(useLocation); -const mockedUseNavigate = jest.mocked(useNavigate); +const mockReplaceUrl = jest.mocked(replaceUrlWithoutNavigation); describe('useWidgetBuilderState', () => { - let mockNavigate!: jest.Mock; beforeEach(() => { - mockNavigate = jest.fn(); - mockedUseNavigate.mockReturnValue(mockNavigate); jest.useFakeTimers(); }); @@ -71,17 +68,15 @@ describe('useWidgetBuilderState', () => { jest.runAllTimers(); - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({title: 'new title'}), - }), - expect.anything() + }) ); - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({description: 'new description'}), - }), - expect.anything() + }) ); }); @@ -100,7 +95,7 @@ describe('useWidgetBuilderState', () => { ); }); - expect(mockNavigate).not.toHaveBeenCalled(); + expect(mockReplaceUrl).not.toHaveBeenCalled(); }); describe('display type', () => { @@ -146,11 +141,10 @@ describe('useWidgetBuilderState', () => { jest.runAllTimers(); - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({displayType: DisplayType.AREA}), - }), - expect.anything() + }) ); }); @@ -883,11 +877,10 @@ describe('useWidgetBuilderState', () => { jest.runAllTimers(); - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({dataset: WidgetType.METRICS}), - }), - expect.anything() + }) ); }); @@ -1919,11 +1912,10 @@ describe('useWidgetBuilderState', () => { // If selectedAggregate is undefined in the URL, then the widget builder state // will set the selectedAggregate to the last aggregate expect(result.current.state.selectedAggregate).toBe(1); - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({selectedAggregate: undefined}), - }), - expect.anything() + }) ); }); }); @@ -2049,17 +2041,16 @@ describe('useWidgetBuilderState', () => { jest.runAllTimers(); // yAxis should be cleared - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ yAxis: [], }), - }), - expect.anything() + }) ); // fields should contain the default X-axis (project) plus both aggregates with args - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ field: serializeFields([ @@ -2080,18 +2071,16 @@ describe('useWidgetBuilderState', () => { }, ]), }), - }), - expect.anything() + }) ); // sort should reference the full aggregate string with args - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ sort: ['-per_second(value,my.metric,counter,none)'], }), - }), - expect.anything() + }) ); }); }); @@ -2127,7 +2116,7 @@ describe('useWidgetBuilderState', () => { jest.runAllTimers(); // Should preserve aggregates while updating X-axis - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ field: serializeFields([ @@ -2138,8 +2127,7 @@ describe('useWidgetBuilderState', () => { }, ]), }), - }), - expect.anything() + }) ); }); @@ -2174,13 +2162,12 @@ describe('useWidgetBuilderState', () => { jest.runAllTimers(); // Sort should be reset to first aggregate - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ sort: ['-count()'], }), - }), - expect.anything() + }) ); }); @@ -2215,13 +2202,12 @@ describe('useWidgetBuilderState', () => { jest.runAllTimers(); // Sort should NOT change since it was already on an aggregate - expect(mockNavigate).not.toHaveBeenCalledWith( + expect(mockReplaceUrl).not.toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ sort: expect.anything(), }), - }), - expect.anything() + }) ); }); @@ -2249,7 +2235,7 @@ describe('useWidgetBuilderState', () => { jest.runAllTimers(); // Equation should be preserved as the aggregate - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ field: serializeFields([ @@ -2257,18 +2243,16 @@ describe('useWidgetBuilderState', () => { {kind: FieldValueKind.EQUATION, field: 'count() / 5'}, ]), }), - }), - expect.anything() + }) ); // Sort should use equation[0] alias format - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ sort: ['-equation[0]'], }), - }), - expect.anything() + }) ); }); @@ -2300,7 +2284,7 @@ describe('useWidgetBuilderState', () => { jest.runAllTimers(); // Equation should be preserved in fields - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ field: serializeFields([ @@ -2308,18 +2292,16 @@ describe('useWidgetBuilderState', () => { {kind: FieldValueKind.EQUATION, field: 'count() / 5'}, ]), }), - }), - expect.anything() + }) ); // Sort should NOT be reset since equation[0] is still valid - expect(mockNavigate).not.toHaveBeenCalledWith( + expect(mockReplaceUrl).not.toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ sort: expect.anything(), }), - }), - expect.anything() + }) ); }); @@ -2354,13 +2336,12 @@ describe('useWidgetBuilderState', () => { jest.runAllTimers(); // Sort should be reset to first aggregate since there are no equations in fields - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ sort: ['-count()'], }), - }), - expect.anything() + }) ); }); @@ -2389,7 +2370,7 @@ describe('useWidgetBuilderState', () => { jest.runAllTimers(); // Both the function and equation should be preserved - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ field: serializeFields([ @@ -2401,18 +2382,16 @@ describe('useWidgetBuilderState', () => { {kind: FieldValueKind.EQUATION, field: 'count() / 5'}, ]), }), - }), - expect.anything() + }) ); // Sort should be on the last aggregate (equation) by default - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ sort: ['-equation[0]'], }), - }), - expect.anything() + }) ); }); @@ -2498,7 +2477,7 @@ describe('useWidgetBuilderState', () => { // Each state setter makes a separate navigate call - check each one // Should set default X-axis field and aggregate for new dataset - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ // Errors dataset defaults: title (X-axis) + count_unique(user) (aggregate) @@ -2510,24 +2489,21 @@ describe('useWidgetBuilderState', () => { }, ]), }), - }), - expect.anything() + }) ); - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ sort: ['-count_unique(user)'], }), - }), - expect.anything() + }) ); - expect(mockNavigate).toHaveBeenCalledWith( + expect(mockReplaceUrl).toHaveBeenCalledWith( expect.objectContaining({ query: expect.objectContaining({ limit: 20, }), - }), - expect.anything() + }) ); }); }); @@ -2664,7 +2640,7 @@ describe('useWidgetBuilderState', () => { expect(result.current.state.textContent!).toBe('new text content'); // Text content must not be written to the URL to avoid excessive URL length - expect(mockNavigate).not.toHaveBeenCalled(); + expect(mockReplaceUrl).not.toHaveBeenCalled(); }); }); }); diff --git a/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.tsx b/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.tsx index b6ccece1ce4c..b61a7b42c73d 100644 --- a/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.tsx +++ b/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.tsx @@ -1,5 +1,8 @@ -import {useCallback, useMemo} from 'react'; +import {useEffect, useMemo, useState, useSyncExternalStore} from 'react'; +import * as Sentry from '@sentry/react'; +import debounce from 'lodash/debounce'; import partition from 'lodash/partition'; +import * as qs from 'query-string'; import {defined} from 'sentry/utils/defined'; import { @@ -18,8 +21,9 @@ import { decodeScalar, decodeSorts, } from 'sentry/utils/queryString'; -import {useQueryParamState} from 'sentry/utils/url/useQueryParamState'; -import {useSessionStorage} from 'sentry/utils/useSessionStorage'; +import {replaceUrlWithoutNavigation} from 'sentry/utils/url/replaceUrlWithoutNavigation'; +import {useLocation} from 'sentry/utils/useLocation'; +import {readStorageValue, writeStorageValue} from 'sentry/utils/useSessionStorage'; import {getDatasetConfig} from 'sentry/views/dashboards/datasetConfig/base'; import { DEFAULT_CATEGORICAL_BAR_LIMIT, @@ -299,142 +303,226 @@ function fixupTableSortOnRemoval( : []; } -export function useWidgetBuilderState(): { +const URL_UPDATE_DEBOUNCE = 300; + +type QueryParams = Record; + +type SerializedUrlParams = Record; + +export interface WidgetBuilderStore { + /** + * Applies an action to the widget builder state and (unless `updateUrl` is + * false) queues the corresponding query param updates for the URL. + */ dispatch: (action: WidgetAction, options?: WidgetBuilderStateActionOptions) => void; - state: WidgetBuilderState; -} { - const [title, setTitle] = useQueryParamState({fieldName: 'title'}); - const [description, setDescription] = useQueryParamState({ - fieldName: 'description', - }); - const [displayType, setDisplayType] = useQueryParamState({ - fieldName: 'displayType', - deserializer: deserializeDisplayType, - }); - const [dataset, setDataset] = useQueryParamState({ - fieldName: 'dataset', - deserializer: deserializeDataset, - }); - const [fields, setFields] = useQueryParamState({ - fieldName: 'field', - decoder: decodeList, - deserializer: deserializeFields, - serializer: serializeFields, - }); - const [yAxis, setYAxis] = useQueryParamState({ - fieldName: 'yAxis', - decoder: decodeList, - deserializer: deserializeFields, - serializer: serializeFields, - }); - const [query, setQuery] = useQueryParamState({ - fieldName: 'query', - decoder: decodeList, - deserializer: deserializeQuery, - }); - const [sort, setSort] = useQueryParamState({ - fieldName: 'sort', - decoder: decodeSorts, - deserializer: deserializeSorts(dataset), - serializer: serializeSorts(dataset), - }); - const [limit, setLimit] = useQueryParamState({ - fieldName: 'limit', - decoder: decodeScalar, - deserializer: deserializeLimit, - }); - const [legendAlias, setLegendAlias] = useQueryParamState({ - fieldName: 'legendAlias', - decoder: decodeList, - }); - const [legendType, setLegendType] = useQueryParamState({ - fieldName: 'legendType', - deserializer: deserializeLegendType, - }); - const [selectedAggregate, setSelectedAggregate] = useQueryParamState({ - fieldName: 'selectedAggregate', - decoder: decodeScalar, - deserializer: deserializeSelectedAggregate, - }); - const [thresholds, setThresholds] = useQueryParamState({ - fieldName: 'thresholds', - decoder: decodeScalar, - deserializer: deserializeThresholds, - serializer: serializeThresholds, - }); - const [linkedDashboards, setLinkedDashboards] = useQueryParamState({ - fieldName: 'linkedDashboards', - decoder: decodeList, - deserializer: deserializeLinkedDashboards, - serializer: serializeLinkedDashboards, - }); - const [axisRange, setAxisRange] = useQueryParamState({ - fieldName: 'axisRange', - decoder: decodeScalar, - deserializer: getAxisRange, - }); - const [textContent, setTextContent, _removeTextContent] = useSessionStorage< - string | undefined - >(WIDGET_BUILDER_SESSION_STORAGE_KEY_MAP.textContent.key, undefined); + /** + * Returns the current state snapshot. The snapshot object is only replaced + * when the state changes, so it is safe to use with `useSyncExternalStore`. + */ + getState: () => WidgetBuilderState; + /** + * Returns the serialized widget builder query params that have been written + * (or are pending a debounced write) to the URL by this store. + */ + getUrlParams: () => SerializedUrlParams; + subscribe: (listener: () => void) => () => void; + /** + * Re-asserts the store's query params onto the current browser URL. Used + * after router navigations (e.g. page filter changes), which rebuild the + * URL from the router's location and would otherwise drop the params this + * store wrote with `replaceUrlWithoutNavigation`. + */ + syncUrlParams: () => void; + /** + * Cancels any pending URL updates. Call on unmount. + */ + teardown: () => void; +} + +/** + * Hydrates the widget builder state from query params, mirroring the decoding + * behavior `useLocationQuery`/`useQueryParamState` previously applied to each + * field. + */ +function hydrateWidgetBuilderState(query: QueryParams): WidgetBuilderState { + // The dataset is decoded first because decoding the sorts depends on it + const dataset = deserializeDataset(decodeScalar(query.dataset, '')); + return { + title: decodeScalar(query.title, ''), + description: decodeScalar(query.description, ''), + displayType: deserializeDisplayType(decodeScalar(query.displayType, '')), + dataset, + fields: deserializeFields(decodeList(query.field)), + yAxis: deserializeFields(decodeList(query.yAxis)), + query: deserializeQuery(decodeList(query.query)), + sort: deserializeSorts(dataset)(decodeSorts(query.sort)), + limit: deserializeLimit(decodeScalar(query.limit, '')), + legendAlias: decodeList(query.legendAlias), + legendType: deserializeLegendType(decodeScalar(query.legendType, '')), + selectedAggregate: deserializeSelectedAggregate( + decodeScalar(query.selectedAggregate, '') + ), + thresholds: deserializeThresholds(decodeScalar(query.thresholds, '')), + linkedDashboards: deserializeLinkedDashboards(decodeList(query.linkedDashboards)), + axisRange: getAxisRange(decodeScalar(query.axisRange, '')), + textContent: readStorageValue( + WIDGET_BUILDER_SESSION_STORAGE_KEY_MAP.textContent.key, + undefined + ), + }; +} + +/** + * Builds the public state snapshot from the raw state, deriving the default + * selected aggregate for big number and categorical bar widgets. + */ +function buildStateSnapshot(rawState: WidgetBuilderState): WidgetBuilderState { + const {displayType, fields, selectedAggregate} = rawState; + return { + ...rawState, + // The selected aggregate is the last aggregate for big number and categorical bar widgets + // if it hasn't been explicitly set. + // For categorical bar, only count aggregate fields (FUNCTION/EQUATION), not the X-axis FIELD column + selectedAggregate: + displayType === DisplayType.BIG_NUMBER && defined(fields) && fields.length > 1 + ? (selectedAggregate ?? fields.length - 1) + : displayType === DisplayType.CATEGORICAL_BAR && defined(fields) + ? (() => { + const aggregateCount = fields.filter( + f => + f.kind === FieldValueKind.FUNCTION || f.kind === FieldValueKind.EQUATION + ).length; + return aggregateCount > 1 + ? Math.min(selectedAggregate ?? aggregateCount - 1, aggregateCount - 1) + : undefined; + })() + : undefined, + }; +} + +/** + * Creates a store holding the widget builder state. + * + * State updates are pushed to subscribers synchronously, and the + * corresponding query param updates are written to the URL on a debounce via + * `replaceUrlWithoutNavigation`, so persisting the state never re-renders + * router location subscribers (e.g. the dashboard underneath the builder). + */ +export function createWidgetBuilderStore(initialQuery: QueryParams): WidgetBuilderStore { + let rawState = hydrateWidgetBuilderState(initialQuery); + let snapshot = buildStateSnapshot(rawState); + // Mirror of the serialized params this store has written (or queued) for + // the URL. Used to re-assert them after router navigations and to know + // whether the URL describes a widget. + let urlParams: SerializedUrlParams = {}; + let pendingUrlUpdates: SerializedUrlParams = {}; + const listeners = new Set<() => void>(); + + const flushUrlUpdates = () => { + if (Object.keys(pendingUrlUpdates).length === 0) { + return; + } + + replaceUrlWithoutNavigation({ + pathname: window.location.pathname, + query: { + ...qs.parse(window.location.search), + ...pendingUrlUpdates, + }, + }); + pendingUrlUpdates = {}; + }; - const state = useMemo( - () => ({ - title, + const debouncedFlushUrlUpdates = debounce(flushUrlUpdates, URL_UPDATE_DEBOUNCE); + + const dispatch = (action: WidgetAction, options?: WidgetBuilderStateActionOptions) => { + // Reads within the reducer see the state as of the start of the dispatch, + // matching the render-closure reads of the previous hook implementation + const { description, - textContent, - displayType, - dataset, - fields, - yAxis, - query, - sort, - limit, - legendAlias, - legendType, - thresholds, - linkedDashboards, - axisRange, - // The selected aggregate is the last aggregate for big number and categorical bar widgets - // if it hasn't been explicitly set. - // For categorical bar, only count aggregate fields (FUNCTION/EQUATION), not the X-axis FIELD column - selectedAggregate: - displayType === DisplayType.BIG_NUMBER && defined(fields) && fields.length > 1 - ? (selectedAggregate ?? fields.length - 1) - : displayType === DisplayType.CATEGORICAL_BAR && defined(fields) - ? (() => { - const aggregateCount = fields.filter( - f => - f.kind === FieldValueKind.FUNCTION || - f.kind === FieldValueKind.EQUATION - ).length; - return aggregateCount > 1 - ? Math.min(selectedAggregate ?? aggregateCount - 1, aggregateCount - 1) - : undefined; - })() - : undefined, - }), - [ - title, displayType, - textContent, - description, dataset, fields, yAxis, query, sort, limit, - legendAlias, legendType, - thresholds, - linkedDashboards, - axisRange, selectedAggregate, - ] - ); + linkedDashboards, + } = rawState; + + const changes: Partial = {}; + const urlUpdates: SerializedUrlParams = {}; + + // Mirrors the update behavior of the previous `useQueryParamState` + // setters: stage the state change and, unless `updateUrl` is false, queue + // the serialized value for the URL (`undefined` removes the param). + function makeSetter( + stateKey: K, + paramName: string, + serializer?: (value: NonNullable) => string | string[] + ) { + return ( + value: WidgetBuilderState[K], + setterOptions: WidgetBuilderStateActionOptions = {updateUrl: true} + ) => { + changes[stateKey] = value; + + if (!setterOptions?.updateUrl) { + return; + } + + if (!defined(value)) { + urlUpdates[paramName] = undefined; + } else if (serializer) { + urlUpdates[paramName] = serializer(value); + } else if ( + ['string', 'number', 'boolean'].includes(typeof value) || + Array.isArray(value) + ) { + urlUpdates[paramName] = value as any; + } else { + Sentry.captureException( + new Error( + 'useWidgetBuilderState: value is not a primitive value and not provided a serializer' + ) + ); + urlUpdates[paramName] = undefined; + } + }; + } - const dispatch = useCallback( - (action: WidgetAction, options?: WidgetBuilderStateActionOptions) => { + const setTitle = makeSetter('title', 'title'); + const setDescription = makeSetter('description', 'description'); + const setDisplayType = makeSetter('displayType', 'displayType'); + const setDataset = makeSetter('dataset', 'dataset'); + const setFields = makeSetter('fields', 'field', serializeFields); + const setYAxis = makeSetter('yAxis', 'yAxis', serializeFields); + const setQuery = makeSetter('query', 'query'); + const setSort = makeSetter('sort', 'sort', serializeSorts(dataset)); + const setLimit = makeSetter('limit', 'limit'); + const setLegendAlias = makeSetter('legendAlias', 'legendAlias'); + const setLegendType = makeSetter('legendType', 'legendType'); + const setSelectedAggregate = makeSetter('selectedAggregate', 'selectedAggregate'); + const setThresholds = makeSetter('thresholds', 'thresholds', serializeThresholds); + const setLinkedDashboards = makeSetter( + 'linkedDashboards', + 'linkedDashboards', + serializeLinkedDashboards + ); + const setAxisRange = makeSetter('axisRange', 'axisRange'); + + // Text content is persisted in session storage instead of the URL to + // avoid excessively long URLs + const setTextContent = (value: string | undefined) => { + changes.textContent = value; + writeStorageValue(WIDGET_BUILDER_SESSION_STORAGE_KEY_MAP.textContent.key, value); + }; + + // The reducer is wrapped in a function so early `return`s in the switch + // still fall through to the commit below + const run = () => { const currentDatasetConfig = getDatasetConfig(dataset); switch (action.type) { case BuilderStateAction.SET_TITLE: @@ -1185,44 +1273,86 @@ export function useWidgetBuilderState(): { break; } } - }, - [ - dataset, - setTitle, - setDescription, - setQuery, - displayType, - setLimit, - setLegendAlias, - setLegendType, - setSelectedAggregate, - fields, - setDataset, - setDisplayType, - setSort, - setAxisRange, - setThresholds, - yAxis, - setLinkedDashboards, - setTextContent, - setYAxis, - setFields, - sort, - query, - description, - limit, - legendType, - linkedDashboards, - selectedAggregate, - ] - ); + }; + + run(); + + if (Object.keys(changes).length > 0) { + rawState = {...rawState, ...changes}; + snapshot = buildStateSnapshot(rawState); + } + + if (Object.keys(urlUpdates).length > 0) { + urlParams = {...urlParams, ...urlUpdates}; + pendingUrlUpdates = {...pendingUrlUpdates, ...urlUpdates}; + debouncedFlushUrlUpdates(); + } + + if (Object.keys(changes).length > 0) { + listeners.forEach(listener => listener()); + } + }; return { - state, dispatch, + getState: () => snapshot, + getUrlParams: () => urlParams, + subscribe: (listener: () => void) => { + listeners.add(listener); + return () => { + listeners.delete(listener); + }; + }, + syncUrlParams: () => { + if ( + Object.keys(urlParams).length === 0 && + Object.keys(pendingUrlUpdates).length === 0 + ) { + return; + } + replaceUrlWithoutNavigation({ + pathname: window.location.pathname, + query: { + ...qs.parse(window.location.search), + ...urlParams, + ...pendingUrlUpdates, + }, + }); + }, + teardown: () => { + debouncedFlushUrlUpdates.cancel(); + pendingUrlUpdates = {}; + }, }; } +export function useWidgetBuilderState(): { + dispatch: (action: WidgetAction, options?: WidgetBuilderStateActionOptions) => void; + state: WidgetBuilderState; + store: WidgetBuilderStore; +} { + const location = useLocation(); + + const [store] = useState(() => { + // On widget builder routes, window.location may be fresher than the + // router's location because the store writes the URL without notifying + // the router. Outside the browser builder route (e.g. tests), fall back + // to the router's location alone. + const windowQuery = window.location.pathname.includes('/widget-builder/') + ? qs.parse(window.location.search) + : {}; + return createWidgetBuilderStore({...location.query, ...windowQuery}); + }); + + // Cancel pending URL updates on unmount; the widget builder cleans up its + // own params when it closes + useEffect(() => () => store.teardown(), [store]); + + const state = useSyncExternalStore(store.subscribe, store.getState); + + return useMemo(() => ({state, dispatch: store.dispatch, store}), [state, store]); +} + /** * Decodes the display type from the query params * Returns the default display type if the value is not a valid display type diff --git a/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderTraceItemConfig.ts b/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderTraceItemConfig.ts index 4beaa55bbdd4..9ddba69dbbf6 100644 --- a/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderTraceItemConfig.ts +++ b/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderTraceItemConfig.ts @@ -1,7 +1,7 @@ import {defined} from 'sentry/utils/defined'; import {useOrganization} from 'sentry/utils/useOrganization'; import {WidgetType} from 'sentry/views/dashboards/types'; -import {useWidgetBuilderContext} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; +import {useWidgetBuilderStateSlice} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext'; import {useTraceMetricMultiMetricSelection} from 'sentry/views/dashboards/widgetBuilder/hooks/useTraceMetricMultiMetricSelection'; import { extractTraceMetricFromColumn, @@ -14,7 +14,7 @@ import {createTraceMetricFilter} from 'sentry/views/explore/metrics/utils'; import {TraceItemDataset} from 'sentry/views/explore/types'; export function useWidgetBuilderTraceItemConfig(): TraceItemAttributeConfig { - const {state} = useWidgetBuilderContext(); + const state = useWidgetBuilderStateSlice('dataset', 'displayType', 'fields', 'yAxis'); const organization = useOrganization(); const hasMultiMetricSelection = useTraceMetricMultiMetricSelection();