Skip to content

Commit 2ef1637

Browse files
authored
feat(widget-builder): Do not trigger URL updates for every keystroke (#98903)
This is one of a series of improvements I'm trying to make to the widget builder state management. The goal of this PR is to prevent keystrokes in fields such as the title, description, or the alias input fields from immediately trigging a re-render as a result from updating the URL params. Those changes should only be pushed to the URL during the `onBlur` when a user clicks out of a field.
1 parent e561956 commit 2ef1637

File tree

7 files changed

+194
-96
lines changed

7 files changed

+194
-96
lines changed

static/app/utils/url/useQueryParamState.tsx

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,10 @@ type UseQueryParamStateProps<T> =
3232
| UseQueryParamStateWithListDecoder<T>
3333
| UseQueryParamStateWithSortsDecoder<T>;
3434

35+
type UseQueryParamStateOptions = {
36+
updateUrl?: boolean;
37+
};
38+
3539
/**
3640
* Hook to manage a state that is synced with a query param in the URL
3741
*
@@ -44,7 +48,10 @@ export function useQueryParamState<T = string>({
4448
decoder,
4549
deserializer,
4650
serializer,
47-
}: UseQueryParamStateProps<T>): [T | undefined, (newField: T | undefined) => void] {
51+
}: UseQueryParamStateProps<T>): [
52+
T | undefined,
53+
(newField: T | undefined, options?: UseQueryParamStateOptions) => void,
54+
] {
4855
const {batchUrlParamUpdates} = useUrlBatchContext();
4956

5057
// The URL query params give us our initial state
@@ -69,9 +76,13 @@ export function useQueryParamState<T = string>({
6976
});
7077

7178
const updateField = useCallback(
72-
(newField: T | undefined) => {
79+
(newField: T | undefined, options: UseQueryParamStateOptions = {updateUrl: true}) => {
7380
setLocalState(newField);
7481

82+
if (!options?.updateUrl) {
83+
return;
84+
}
85+
7586
if (!defined(newField)) {
7687
batchUrlParamUpdates({[fieldName]: undefined});
7788
} else if (serializer) {

static/app/views/dashboards/widgetBuilder/components/nameAndDescFields.spec.tsx

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,9 @@ describe('WidgetBuilder', () => {
4343
);
4444

4545
await userEvent.type(await screen.findByPlaceholderText('Name'), 'some name');
46+
47+
// trigger blur
48+
await userEvent.tab();
4649
expect(mockNavigate).toHaveBeenLastCalledWith(
4750
expect.objectContaining({
4851
...router.location,
@@ -57,6 +60,9 @@ describe('WidgetBuilder', () => {
5760
await screen.findByPlaceholderText('Description'),
5861
'some description'
5962
);
63+
64+
// trigger blur
65+
await userEvent.tab();
6066
expect(mockNavigate).toHaveBeenLastCalledWith(
6167
expect.objectContaining({
6268
...router.location,

static/app/views/dashboards/widgetBuilder/components/nameAndDescFields.tsx

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -46,10 +46,19 @@ function WidgetBuilderNameAndDescription({
4646
value={state.title}
4747
onChange={(newTitle: any) => {
4848
// clear error once user starts typing
49-
setError?.({...error, title: undefined});
50-
dispatch({type: BuilderStateAction.SET_TITLE, payload: newTitle});
49+
if (error?.title) {
50+
setError?.({...error, title: undefined});
51+
}
52+
dispatch(
53+
{type: BuilderStateAction.SET_TITLE, payload: newTitle},
54+
{updateUrl: false}
55+
);
5156
}}
52-
onBlur={() => {
57+
onBlur={value => {
58+
dispatch(
59+
{type: BuilderStateAction.SET_TITLE, payload: value},
60+
{updateUrl: true}
61+
);
5362
trackAnalytics('dashboards_views.widget_builder.change', {
5463
from: source,
5564
widget_type: state.dataset ?? '',
@@ -85,9 +94,19 @@ function WidgetBuilderNameAndDescription({
8594
rows={4}
8695
value={state.description}
8796
onChange={e => {
88-
dispatch({type: BuilderStateAction.SET_DESCRIPTION, payload: e.target.value});
97+
dispatch(
98+
{type: BuilderStateAction.SET_DESCRIPTION, payload: e.target.value},
99+
{updateUrl: false}
100+
);
89101
}}
90-
onBlur={() => {
102+
onBlur={e => {
103+
dispatch(
104+
{
105+
type: BuilderStateAction.SET_DESCRIPTION,
106+
payload: e.target.value,
107+
},
108+
{updateUrl: true}
109+
);
91110
trackAnalytics('dashboards_views.widget_builder.change', {
92111
from: source,
93112
widget_type: state.dataset ?? '',

static/app/views/dashboards/widgetBuilder/components/queryFilterBuilder.tsx

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -203,18 +203,34 @@ function WidgetBuilderQueryFilterBuilder({
203203
{canHaveAlias && (
204204
<LegendAliasInput
205205
type="text"
206-
name="name"
206+
name="alias"
207207
placeholder={t('Legend Alias')}
208208
value={state.legendAlias?.[index] || ''}
209209
onChange={e => {
210-
dispatch({
211-
type: BuilderStateAction.SET_LEGEND_ALIAS,
212-
payload: state.legendAlias?.length
213-
? state.legendAlias?.map((q, i) => (i === index ? e.target.value : q))
214-
: [e.target.value],
215-
});
210+
dispatch(
211+
{
212+
type: BuilderStateAction.SET_LEGEND_ALIAS,
213+
payload: state.legendAlias?.length
214+
? state.legendAlias?.map((q, i) =>
215+
i === index ? e.target.value : q
216+
)
217+
: [e.target.value],
218+
},
219+
{updateUrl: false}
220+
);
216221
}}
217-
onBlur={() => {
222+
onBlur={e => {
223+
dispatch(
224+
{
225+
type: BuilderStateAction.SET_LEGEND_ALIAS,
226+
payload: state.legendAlias?.length
227+
? state.legendAlias?.map((q, i) =>
228+
i === index ? e.target.value : q
229+
)
230+
: [e.target.value],
231+
},
232+
{updateUrl: true}
233+
);
218234
trackAnalytics('dashboards_views.widget_builder.change', {
219235
builder_version: WidgetBuilderVersion.SLIDEOUT,
220236
field: 'filter.alias',

static/app/views/dashboards/widgetBuilder/components/visualize/index.tsx

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -700,17 +700,25 @@ function Visualize({error, setError}: VisualizeProps) {
700700
<FieldExtras isChartWidget={isChartWidget || isBigNumberWidget}>
701701
{!isChartWidget && !isBigNumberWidget && (
702702
<LegendAliasInput
703-
type="text"
704-
name="name"
703+
name="alias"
705704
placeholder={t('Add Alias')}
706705
value={field.alias ?? ''}
707706
disabled={disableTransactionWidget}
708707
onChange={e => {
709708
const newFields = cloneDeep(fields);
710709
newFields[index]!.alias = e.target.value;
711-
dispatch({type: updateAction, payload: newFields});
710+
dispatch(
711+
{type: updateAction, payload: newFields},
712+
{updateUrl: false}
713+
);
712714
}}
713-
onBlur={() => {
715+
onBlur={e => {
716+
const newFields = cloneDeep(fields);
717+
newFields[index]!.alias = e.target.value;
718+
dispatch(
719+
{type: updateAction, payload: newFields},
720+
{updateUrl: true}
721+
);
714722
trackAnalytics('dashboards_views.widget_builder.change', {
715723
builder_version: WidgetBuilderVersion.SLIDEOUT,
716724
field: 'visualize.legendAlias',

static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.spec.tsx

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,24 @@ describe('useWidgetBuilderState', () => {
8484
);
8585
});
8686

87+
it('does not update the url when the updateUrl option is false', () => {
88+
const {result} = renderHook(() => useWidgetBuilderState(), {
89+
wrapper: WidgetBuilderProvider,
90+
});
91+
92+
act(() => {
93+
result.current.dispatch(
94+
{
95+
type: BuilderStateAction.SET_TITLE,
96+
payload: 'new title',
97+
},
98+
{updateUrl: false}
99+
);
100+
});
101+
102+
expect(mockNavigate).not.toHaveBeenCalled();
103+
});
104+
87105
describe('display type', () => {
88106
it('returns the display type from the query params', () => {
89107
mockedUsedLocation.mockReturnValue(

0 commit comments

Comments
 (0)