Skip to content

Commit fa01ed4

Browse files
JoshuaKGoldbergclaude
JoshuaKGoldberg
and
claude
authored
ref(exports): refactor useDataExport to use a mutation hook (#117453)
Modernizes `useDataExport` from a hand-rolled `useCallback` + `api.requestPromise` implementation into a TanStack `useMutation` hook. Finally doing the cleanup I'd wanted to 8 weeks ago! 🙌 🧹 There should be no practical behavior changes, just code cleanup. Closes LOGS-698 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 2338f81 commit fa01ed4

7 files changed

Lines changed: 103 additions & 153 deletions

File tree

‎static/app/components/exports/dataExport.tsx‎

Lines changed: 6 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import {useEffect, useRef, useState} from 'react';
1+
import {useEffect} from 'react';
22

33
import {Button} from '@sentry/scraps/button';
44

@@ -41,35 +41,17 @@ export function DataExport({
4141
overrideFeatureFlags,
4242
onClick,
4343
}: DataExportProps): React.ReactElement {
44-
const unmountedRef = useRef(false);
45-
const [inProgress, setInProgress] = useState(false);
46-
const handleDataExport = useDataExport({
47-
unmountedRef,
48-
inProgressCallback: setInProgress,
49-
});
44+
const {mutate, reset, isPending, isSuccess} = useDataExport();
45+
const inProgress = isPending || isSuccess;
5046

5147
// We clear the indicator if export props change so that the user
5248
// can fire another export without having to wait for the previous one to finish.
5349
useEffect(() => {
54-
if (inProgress) {
55-
setInProgress(false);
56-
}
57-
// We are skipping the inProgress dependency because it would have fired on each handleDataExport
58-
// call and would have immediately turned off the value giving users no feedback on their click action.
59-
// An alternative way to handle this would have probably been to key the component by payload/queryType,
60-
// but that seems like it can be a complex object so tracking changes could result in very brittle behavior.
61-
// eslint-disable-next-line react-hooks/exhaustive-deps
62-
}, [payload.queryType, payload.queryInfo]);
63-
64-
// Tracking unmounting of the component to prevent setState call on unmounted component
65-
useEffect(() => {
66-
return () => {
67-
unmountedRef.current = true;
68-
};
69-
}, []);
50+
reset();
51+
}, [payload.queryType, payload.queryInfo, reset]);
7052

7153
const handleClick = () => {
72-
handleDataExport(payload);
54+
mutate(payload);
7355
onClick?.();
7456
};
7557

‎static/app/components/exports/useDataExport.spec.ts‎

Lines changed: 27 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -33,30 +33,25 @@ describe('useDataExport', () => {
3333
});
3434

3535
it('should display default error message on failure when none is provided', async () => {
36-
const inProgressCallback = jest.fn();
37-
3836
MockApiClient.addMockResponse({
3937
...requestBase,
4038
statusCode: 400,
4139
});
4240

43-
const {result} = renderHookWithProviders(() => useDataExport({inProgressCallback}), {
41+
const {result} = renderHookWithProviders(() => useDataExport(), {
4442
organization: mockAuthorizedOrg,
4543
});
4644

47-
await result.current({...mockPayload});
45+
result.current.mutate({...mockPayload});
4846

4947
await waitFor(() => {
5048
expect(addErrorMessage).toHaveBeenCalledWith(
5149
"We tried our hardest, but we couldn't export your data. Try waiting a minute then giving it another go."
5250
);
5351
});
54-
55-
expect(inProgressCallback).toHaveBeenCalledWith(false);
5652
});
5753

5854
it('should display the provided error message on failure when one is provided', async () => {
59-
const inProgressCallback = jest.fn();
6055
const detail = 'Oh no!';
6156

6257
MockApiClient.addMockResponse({
@@ -65,34 +60,15 @@ describe('useDataExport', () => {
6560
body: {detail},
6661
});
6762

68-
const {result} = renderHookWithProviders(() => useDataExport({inProgressCallback}), {
63+
const {result} = renderHookWithProviders(() => useDataExport(), {
6964
organization: mockAuthorizedOrg,
7065
});
7166

72-
await result.current({...mockPayload});
67+
result.current.mutate({...mockPayload});
7368

7469
await waitFor(() => {
7570
expect(addErrorMessage).toHaveBeenCalledWith(detail);
7671
});
77-
78-
expect(inProgressCallback).toHaveBeenCalledWith(false);
79-
});
80-
81-
it('should not add an error message when unmountedRef is already true', async () => {
82-
MockApiClient.addMockResponse({
83-
...requestBase,
84-
statusCode: 400,
85-
});
86-
87-
const unmountedRef = {current: true};
88-
89-
const {result} = renderHookWithProviders(() => useDataExport({unmountedRef}), {
90-
organization: mockAuthorizedOrg,
91-
});
92-
93-
await result.current({...mockPayload});
94-
95-
expect(addErrorMessage).not.toHaveBeenCalled();
9672
});
9773

9874
it('should notify when export is queued (201, no fileName)', async () => {
@@ -106,7 +82,7 @@ describe('useDataExport', () => {
10682
organization: mockAuthorizedOrg,
10783
});
10884

109-
await result.current({...mockPayload});
85+
result.current.mutate({...mockPayload});
11086

11187
await waitFor(() => {
11288
expect(addSuccessMessage).toHaveBeenCalledWith(
@@ -126,7 +102,7 @@ describe('useDataExport', () => {
126102
organization: mockAuthorizedOrg,
127103
});
128104

129-
await result.current({...mockPayload});
105+
result.current.mutate({...mockPayload});
130106

131107
await waitFor(() => {
132108
expect(addSuccessMessage).toHaveBeenCalledWith(
@@ -146,7 +122,7 @@ describe('useDataExport', () => {
146122
organization: mockAuthorizedOrg,
147123
});
148124

149-
await result.current({format: 'csv', ...mockPayload});
125+
result.current.mutate({format: 'csv', ...mockPayload});
150126

151127
await waitFor(() => {
152128
expect(downloadFromHref).toHaveBeenCalledWith(
@@ -171,41 +147,43 @@ describe('useDataExport', () => {
171147
organization: mockAuthorizedOrg,
172148
});
173149

174-
await result.current({
150+
result.current.mutate({
175151
format: 'csv',
176152
queryInfo: mockPayload.queryInfo,
177153
queryType: mockPayload.queryType,
178154
limit: 10_000,
179155
});
180156

181-
expect(exportMock).toHaveBeenCalledWith('/organizations/org-slug/data-export/', {
182-
data: {
183-
format: 'csv',
184-
query_type: mockPayload.queryType,
185-
query_info: mockPayload.queryInfo,
186-
limit: 10_000,
187-
},
188-
error: expect.any(Function),
189-
method: 'POST',
190-
success: expect.any(Function),
157+
await waitFor(() => {
158+
expect(exportMock).toHaveBeenCalledWith('/organizations/org-slug/data-export/', {
159+
data: {
160+
format: 'csv',
161+
query_type: mockPayload.queryType,
162+
query_info: mockPayload.queryInfo,
163+
limit: 10_000,
164+
},
165+
error: expect.any(Function),
166+
method: 'POST',
167+
success: expect.any(Function),
168+
});
191169
});
192170
});
193171

194-
it('should call inProgressCallback with true when an export starts', async () => {
195-
const inProgressCallback = jest.fn();
196-
172+
it('should settle into a success state once an export is queued', async () => {
197173
MockApiClient.addMockResponse({
198174
...requestBase,
199175
statusCode: 201,
200176
body: {id: 721},
201177
});
202178

203-
const {result} = renderHookWithProviders(() => useDataExport({inProgressCallback}), {
179+
const {result} = renderHookWithProviders(() => useDataExport(), {
204180
organization: mockAuthorizedOrg,
205181
});
206182

207-
const exportPromise = result.current({...mockPayload});
208-
expect(inProgressCallback).toHaveBeenCalledWith(true);
209-
await exportPromise;
183+
result.current.mutate({...mockPayload});
184+
185+
await waitFor(() => {
186+
expect(result.current.isSuccess).toBe(true);
187+
});
210188
});
211189
});

‎static/app/components/exports/useDataExport.ts‎

Lines changed: 47 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,14 @@
1-
import {useCallback} from 'react';
1+
import {useMutation} from '@tanstack/react-query';
22

33
import type {EventQuery} from 'sentry/actionCreators/events';
44
import {addErrorMessage, addSuccessMessage} from 'sentry/actionCreators/indicator';
55
import {t} from 'sentry/locale';
6-
import type {ResponseMeta} from 'sentry/types/api';
6+
import type {ApiResult, ResponseMeta} from 'sentry/types/api';
77
import {getApiUrl} from 'sentry/utils/api/getApiUrl';
88
import type {LocationQuery} from 'sentry/utils/discover/eventView';
99
import {downloadFromHref} from 'sentry/utils/downloadFromHref';
10+
import {fetchMutation} from 'sentry/utils/queryClient';
1011
import {RequestError} from 'sentry/utils/requestError/requestError';
11-
import {useApi} from 'sentry/utils/useApi';
1212
import {useOrganization} from 'sentry/utils/useOrganization';
1313
import {createLogDownloadFilename} from 'sentry/views/explore/logs/createLogDownloadFilename';
1414
import type {TraceItemDataset} from 'sentry/views/explore/types';
@@ -75,11 +75,6 @@ export type DataExportPayload =
7575
| DiscoverExportPayload
7676
| ExploreExportPayload;
7777

78-
interface UseDataExportOptions {
79-
inProgressCallback?: (inProgress: boolean) => void;
80-
unmountedRef?: React.RefObject<boolean>;
81-
}
82-
8378
interface DataExportData {
8479
checksum: string | null;
8580
dateCreated: string;
@@ -118,63 +113,50 @@ function handleDataExportResponse(
118113
addSuccessMessage(t("Downloading '%s' to your browser.", data.fileName));
119114
}
120115

121-
/**
122-
* @todo(LOGS-698): Modernize this into using a useApiQuery call.
123-
*/
124-
export function useDataExport({
125-
inProgressCallback,
126-
unmountedRef,
127-
}: UseDataExportOptions = {}) {
116+
export function useDataExport() {
128117
const organization = useOrganization();
129-
const api = useApi();
130-
131-
return useCallback(
132-
async ({format = 'csv', limit, queryInfo, queryType}: DataExportPayload) => {
133-
inProgressCallback?.(true);
134-
135-
const result = await api
136-
.requestPromise(
137-
getApiUrl('/organizations/$organizationIdOrSlug/data-export/', {
138-
path: {organizationIdOrSlug: organization.slug},
139-
}),
140-
{
141-
includeAllArgs: true,
142-
method: 'POST',
143-
data: {
144-
format,
145-
limit,
146-
query_info: queryInfo,
147-
query_type: queryType,
148-
},
149-
}
150-
)
151-
.then(([data, _, response]) => {
152-
if (!unmountedRef?.current) {
153-
handleDataExportResponse(data, format, response, organization.slug);
154-
}
155-
})
156-
.catch(error => {
157-
// If component has unmounted, don't do anything
158-
if (unmountedRef?.current) {
159-
return;
160-
}
161-
if (
162-
error instanceof RequestError &&
163-
typeof error.responseJSON?.detail === 'string'
164-
) {
165-
addErrorMessage(error.responseJSON.detail);
166-
} else {
167-
addErrorMessage(
168-
t(
169-
"We tried our hardest, but we couldn't export your data. Try waiting a minute then giving it another go."
170-
)
171-
);
172-
}
173-
inProgressCallback?.(false);
174-
});
175-
176-
return result!;
118+
119+
return useMutation({
120+
mutationFn: async ({
121+
format = 'csv',
122+
limit,
123+
queryInfo,
124+
queryType,
125+
}: DataExportPayload) => {
126+
const [data, , response] = await fetchMutation<ApiResult>({
127+
url: getApiUrl('/organizations/$organizationIdOrSlug/data-export/', {
128+
path: {organizationIdOrSlug: organization.slug},
129+
}),
130+
options: {
131+
includeAllArgs: true,
132+
},
133+
method: 'POST',
134+
data: {
135+
format,
136+
limit,
137+
query_info: queryInfo,
138+
query_type: queryType,
139+
},
140+
});
141+
142+
return {data: data as DataExportData, format, response};
177143
},
178-
[organization.slug, api, inProgressCallback, unmountedRef]
179-
);
144+
onSuccess: ({data, format, response}) => {
145+
handleDataExportResponse(data, format, response, organization.slug);
146+
},
147+
onError: error => {
148+
if (
149+
error instanceof RequestError &&
150+
typeof error.responseJSON?.detail === 'string'
151+
) {
152+
addErrorMessage(error.responseJSON.detail);
153+
} else {
154+
addErrorMessage(
155+
t(
156+
"We tried our hardest, but we couldn't export your data. Try waiting a minute then giving it another go."
157+
)
158+
);
159+
}
160+
},
161+
});
180162
}

‎static/app/utils/api/apiQueryKey.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ export type QueryKeyEndpointOptions = {
1111
data?: Record<string, unknown>;
1212
headers?: Record<string, string>;
1313
host?: string;
14+
includeAllArgs?: boolean;
1415
method?: RequestMethod;
1516
query?: Record<string, unknown>;
1617
};

‎static/app/utils/queryClient.tsx‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,10 @@ type ApiMutationVariables = {
148148
method: 'PUT' | 'POST' | 'DELETE';
149149
url: string;
150150
data?: Record<string, unknown>;
151-
options?: Pick<QueryKeyEndpointOptions, 'query' | 'headers' | 'host'>;
151+
options?: Pick<
152+
QueryKeyEndpointOptions,
153+
'includeAllArgs' | 'query' | 'headers' | 'host'
154+
>;
152155
};
153156

154157
/**
@@ -160,6 +163,7 @@ export function fetchMutation<TResponseData = unknown>(
160163
const {method, url, options, data} = variables;
161164

162165
return QUERY_API_CLIENT.requestPromise(url, {
166+
includeAllArgs: options?.includeAllArgs,
163167
method,
164168
query: options?.query,
165169
headers: options?.headers,

0 commit comments

Comments
 (0)