Skip to content

Commit 795ce26

Browse files
authored
perf(widget-builder): Store pending URL updates in a ref to prevent re-renders (#101878)
This is a tangential improvement for the Widget Builder that I spotted while trying to improve how fast it opens. Updating a field in the Widget Builder queues two actions 1. Immediately update the widget builder state context 2. Debounce updating the URL to match the state Unfortunately, the second step is done in a context, and queuing a URL update also updates a state, which causes a re-render of the whole builder. This is visible in the React Profiler, you can see two large renders. The former is caused by `WidgetBuilderStateProvider` and the latter by `URLParamBatchProvider`. **Before:** <img width="570" height="53" alt="Screenshot 2025-10-21 at 3 09 05 PM" src="https://github.com/user-attachments/assets/bfc340fb-45c4-4177-a395-f99b09cfd0fa" /> In this PR I'm making a few small changes: 1. The pending URL updates are stored in a `ref` instead of state. Updating the ref does not cause a re-render, nor does it invalidate any callbacks that use the pending updates as a dependency 2. The debounced URL updater is called _immediately_ in the updater function, rather than relying on a `useEffect` triggered by the state update. This is simpler, and is generally a better pattern, even the React docs recommend that events are not expressed as effects. React profiler shows the change, the second large render is missing. In the Chrome profiler, the story is a bit more muddy. For whatever reason, the speedup is not as obvious, it's only about 50ms, which is less than the render duration. I'm chalking this up to discrepancies between the profilers, and profiler overhead. **After:** <img width="270" height="51" alt="Screenshot 2025-10-21 at 3 10 31 PM" src="https://github.com/user-attachments/assets/6cf197ab-6045-454f-a004-0ee623632cc2" /> I also updated the mock for `lodash/debounce`. `debounce` does not work correctly with Jest's timer mocks, we usually mock this out. I'm not sure why the previous tests pass, to be honest!
1 parent e63a314 commit 795ce26

File tree

4 files changed

+101
-41
lines changed

4 files changed

+101
-41
lines changed

static/app/utils/url/testUtils.tsx

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
/**
2+
* A debounce function that has the same API as lodash debounce, but can be advanced via Jest timer mocks.
3+
*/
4+
export function testableDebounce(callback: () => void, delay?: number) {
5+
let timeoutId: ReturnType<typeof setTimeout> | null = null;
6+
7+
const debounced = () => {
8+
if (timeoutId) clearTimeout(timeoutId);
9+
timeoutId = setTimeout(() => callback(), delay);
10+
};
11+
12+
const cancel = () => {
13+
if (timeoutId) clearTimeout(timeoutId);
14+
};
15+
16+
const flush = () => {
17+
if (timeoutId) clearTimeout(timeoutId);
18+
callback();
19+
};
20+
21+
debounced.cancel = cancel;
22+
debounced.flush = flush;
23+
24+
return debounced;
25+
}

static/app/utils/url/urlParamBatchContext.spec.tsx

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
1+
import debounce from 'lodash/debounce';
12
import {LocationFixture} from 'sentry-fixture/locationFixture';
23

3-
import {act, renderHook} from 'sentry-test/reactTestingLibrary';
4+
import {renderHook} from 'sentry-test/reactTestingLibrary';
45

56
import {
67
UrlParamBatchProvider,
@@ -9,20 +10,26 @@ import {
910
import {useLocation} from 'sentry/utils/useLocation';
1011
import {useNavigate} from 'sentry/utils/useNavigate';
1112

13+
import {testableDebounce} from './testUtils';
14+
1215
jest.mock('sentry/utils/useLocation');
1316
jest.mock('sentry/utils/useNavigate');
17+
jest.mock('lodash/debounce');
1418

1519
describe('UrlParamBatchProvider', () => {
1620
let mockNavigate: jest.Mock;
21+
1722
beforeEach(() => {
1823
mockNavigate = jest.fn();
1924
jest.mocked(useNavigate).mockReturnValue(mockNavigate);
25+
jest.mocked(debounce).mockImplementation(testableDebounce);
2026
jest.useFakeTimers();
2127
});
2228

2329
afterEach(() => {
2430
jest.clearAllMocks();
25-
jest.clearAllTimers();
31+
jest.runOnlyPendingTimers();
32+
jest.useRealTimers();
2633
});
2734

2835
it('should batch updates to the URL query params', () => {
@@ -32,14 +39,10 @@ describe('UrlParamBatchProvider', () => {
3239
});
3340
const {batchUrlParamUpdates} = result.current;
3441

35-
act(() => {
36-
batchUrlParamUpdates({foo: 'bar'});
37-
batchUrlParamUpdates({potato: 'test'});
38-
});
42+
batchUrlParamUpdates({foo: 'bar'});
43+
batchUrlParamUpdates({potato: 'test'});
3944

40-
act(() => {
41-
jest.runAllTimers();
42-
});
45+
jest.runAllTimers();
4346

4447
expect(mockNavigate).toHaveBeenCalledTimes(1);
4548
expect(mockNavigate).toHaveBeenCalledWith(
Lines changed: 38 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,6 @@
1-
import {
2-
createContext,
3-
useCallback,
4-
useContext,
5-
useEffect,
6-
useMemo,
7-
useState,
8-
} from 'react';
1+
import {createContext, useCallback, useContext, useEffect, useMemo, useRef} from 'react';
92
import debounce from 'lodash/debounce';
103

11-
import {DEFAULT_DEBOUNCE_DURATION} from 'sentry/constants';
124
import {useLocation} from 'sentry/utils/useLocation';
135
import {useNavigate} from 'sentry/utils/useNavigate';
146

@@ -22,50 +14,67 @@ const BatchContext = createContext<BatchContextType | null>(null);
2214
export function UrlParamBatchProvider({children}: {children: React.ReactNode}) {
2315
const navigate = useNavigate();
2416
const location = useLocation();
25-
const [pendingUpdates, setPendingUpdates] = useState<
26-
Record<string, string | string[] | undefined>
27-
>({});
2817

29-
const batchUrlParamUpdates = useCallback(
30-
(updates: Record<string, string | string[] | undefined>) => {
31-
setPendingUpdates(current => ({...current, ...updates}));
32-
},
33-
[]
34-
);
18+
// Store the pending updates in a `ref`. This way, queuing more updates
19+
// doesn't update any state, so the context doesn't re-render and cause a
20+
// re-render of all its subscribers.
21+
const pendingUpdates = useRef<Record<string, string | string[] | undefined>>({});
3522

3623
const flushUpdates = useCallback(() => {
37-
if (Object.keys(pendingUpdates).length === 0) {
24+
if (Object.keys(pendingUpdates.current).length === 0) {
3825
return;
3926
}
27+
4028
navigate(
4129
{
4230
...location,
4331
query: {
4432
...location.query,
45-
...pendingUpdates,
33+
...pendingUpdates.current,
4634
},
4735
},
4836

4937
// TODO: Use replace until we can sync the state of the widget
5038
// when the user navigates back
5139
{replace: true, preventScrollReset: true}
5240
);
53-
setPendingUpdates({});
54-
}, [location, navigate, pendingUpdates]);
41+
pendingUpdates.current = {};
42+
}, [location, navigate]);
5543

56-
// Debounce URL updates
44+
// Debounced URL updater function
5745
const updateURL = useMemo(
5846
() =>
5947
debounce(() => {
48+
// Flush all current pending URL query parameter updates
6049
flushUpdates();
61-
}, DEFAULT_DEBOUNCE_DURATION),
50+
}, URL_UPDATE_DEBOUNCE),
6251
[flushUpdates]
6352
);
6453

65-
// Trigger the URL updates
54+
const batchUrlParamUpdates = useCallback(
55+
(updates: Record<string, string | string[] | undefined>) => {
56+
// Immediate update the pending URL query parameter updates
57+
pendingUpdates.current = {
58+
...pendingUpdates.current,
59+
...updates,
60+
};
61+
62+
// Immediately calls the debounced URL updater function
63+
updateURL();
64+
},
65+
[updateURL]
66+
);
67+
68+
// Cancel pending changes during `useEffect` cleanup. All the dependencies are
69+
// fairly stable, so this should _only_ happen on unmount. It's important to
70+
// run this on unmount rather than on location change because this context
71+
// might be mounted low in the tree, and might actually get unmounted during a
72+
// location change it should be listening to.
6673
useEffect(() => {
67-
updateURL();
68-
return () => updateURL.cancel();
74+
return () => {
75+
updateURL.cancel();
76+
pendingUpdates.current = {};
77+
};
6978
}, [updateURL]);
7079

7180
return (
@@ -80,3 +89,5 @@ export const useUrlBatchContext = () => {
8089
}
8190
return context;
8291
};
92+
93+
const URL_UPDATE_DEBOUNCE = 300;

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

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,19 @@
1+
import debounce from 'lodash/debounce';
12
import {LocationFixture} from 'sentry-fixture/locationFixture';
23
import {RouterFixture} from 'sentry-fixture/routerFixture';
34

45
import {render, screen, userEvent, waitFor} from 'sentry-test/reactTestingLibrary';
56

67
import {addErrorMessage} from 'sentry/actionCreators/indicator';
8+
import {testableDebounce} from 'sentry/utils/url/testUtils';
79
import {useNavigate} from 'sentry/utils/useNavigate';
810
import WidgetTemplatesList from 'sentry/views/dashboards/widgetBuilder/components/widgetTemplatesList';
911
import {WidgetBuilderProvider} from 'sentry/views/dashboards/widgetBuilder/contexts/widgetBuilderContext';
1012

1113
jest.mock('sentry/utils/useNavigate', () => ({
1214
useNavigate: jest.fn(),
1315
}));
16+
jest.mock('lodash/debounce');
1417

1518
jest.mock('sentry/views/dashboards/widgetLibrary/data', () => ({
1619
getTopNConvertedDefaultWidgets: jest.fn(() => [
@@ -37,9 +40,13 @@ describe('WidgetTemplatesList', () => {
3740
const onSave = jest.fn();
3841

3942
beforeEach(() => {
43+
jest.useFakeTimers();
44+
4045
const mockNavigate = jest.fn();
4146
mockUseNavigate.mockReturnValue(mockNavigate);
4247

48+
jest.mocked(debounce).mockImplementation(testableDebounce);
49+
4350
MockApiClient.addMockResponse({
4451
url: '/organizations/org-slug/dashboards/widgets/',
4552
method: 'POST',
@@ -50,6 +57,8 @@ describe('WidgetTemplatesList', () => {
5057

5158
afterEach(() => {
5259
jest.clearAllMocks();
60+
jest.runOnlyPendingTimers();
61+
jest.useRealTimers();
5362
});
5463

5564
it('should render the widget templates list', async () => {
@@ -72,6 +81,8 @@ describe('WidgetTemplatesList', () => {
7281
});
7382

7483
it('should render buttons when the user clicks on a widget template', async () => {
84+
const user = userEvent.setup({advanceTimers: jest.advanceTimersByTime});
85+
7586
render(
7687
<WidgetBuilderProvider>
7788
<WidgetTemplatesList
@@ -87,13 +98,17 @@ describe('WidgetTemplatesList', () => {
8798
);
8899

89100
const widgetTemplate = await screen.findByText('Duration Distribution');
90-
await userEvent.click(widgetTemplate);
101+
await user.click(widgetTemplate);
102+
103+
jest.runAllTimers();
91104

92105
expect(await screen.findByText('Customize')).toBeInTheDocument();
93106
expect(await screen.findByText('Add to dashboard')).toBeInTheDocument();
94107
});
95108

96109
it('should put widget in url when clicking a template', async () => {
110+
const user = userEvent.setup({advanceTimers: jest.advanceTimersByTime});
111+
97112
const mockNavigate = jest.fn();
98113
mockUseNavigate.mockReturnValue(mockNavigate);
99114

@@ -112,8 +127,10 @@ describe('WidgetTemplatesList', () => {
112127
}
113128
);
114129

115-
const widgetTemplate = await screen.findByText('Duration Distribution');
116-
await userEvent.click(widgetTemplate);
130+
const widgetTemplate = screen.getByText('Duration Distribution');
131+
await user.click(widgetTemplate);
132+
133+
jest.runAllTimers();
117134

118135
expect(mockNavigate).toHaveBeenLastCalledWith(
119136
expect.objectContaining({
@@ -130,6 +147,8 @@ describe('WidgetTemplatesList', () => {
130147
});
131148

132149
it('should show error message when the widget fails to save', async () => {
150+
const user = userEvent.setup({advanceTimers: jest.advanceTimersByTime});
151+
133152
render(
134153
<WidgetBuilderProvider>
135154
<WidgetTemplatesList
@@ -145,9 +164,11 @@ describe('WidgetTemplatesList', () => {
145164
);
146165

147166
const widgetTemplate = await screen.findByText('Duration Distribution');
148-
await userEvent.click(widgetTemplate);
167+
await user.click(widgetTemplate);
168+
169+
await user.click(await screen.findByText('Add to dashboard'));
149170

150-
await userEvent.click(await screen.findByText('Add to dashboard'));
171+
jest.runAllTimers();
151172

152173
await waitFor(() => {
153174
expect(addErrorMessage).toHaveBeenCalledWith('Unable to add widget');

0 commit comments

Comments
 (0)