Skip to content

ref(tsc): refactor self-contained endpoints that need response headers to apiOptions#112347

Merged
TkDodo merged 3 commits intomasterfrom
tkdodo/ref/header-queries-to-apiOptions
Apr 7, 2026
Merged

ref(tsc): refactor self-contained endpoints that need response headers to apiOptions#112347
TkDodo merged 3 commits intomasterfrom
tkdodo/ref/header-queries-to-apiOptions

Conversation

@TkDodo
Copy link
Copy Markdown
Collaborator

@TkDodo TkDodo commented Apr 7, 2026

as discussed in last week’s TSC, we need to start moving endpoints over to apiOptions so that we can re-use caches.

the quickest win would be to implement useApiQuery with apiOptions internally, and to get there, we need to migrate endpoints that now need getResponseHeader over to apiOptions because those headers are exposed differently.

this PR takes the first couple of endpoints that are (mostly) self-contained (no other usages of the url found) and moves them over.

I’ve also exposed selectJsonWithHeaders because the default impl only selects json without headers.

I plan to tackle the other occurrences with an endpoint-by-endpoint approach, as we need to identify all places where an endpoint is used (e.g. invalidateQueries or getApiQueryData or setApiQueryData) and migrate them together.

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Apr 7, 2026
enabled: pathParams !== skipToken,
staleTime,
select: data => data.json,
select: selectJson,
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: inline functions run on every render and perform structualSharing on every render. giving a stable identity makes sure we can avoid that and hit the internal queryObserver cache instead.

return existingData.filter(member => member.id !== variables.memberId);
return {
...existingData,
json: existingData.json.filter(member => member.id !== variables.memberId),
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we get type inference here for existingData which is a huge win

@TkDodo TkDodo marked this pull request as ready for review April 7, 2026 12:13
@TkDodo TkDodo requested review from a team as code owners April 7, 2026 12:13
@TkDodo TkDodo requested a review from ryan953 April 7, 2026 12:13
Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Error handling narrowed to only catch RequestError
    • Added isError check from useQuery to catch all error types, with instanceof narrowing inside for type-safe access to RequestError properties.

Create PR

Or push these changes by commenting:

@cursor push 4536c1d9d5
Preview (4536c1d9d5)
diff --git a/static/app/views/alerts/rules/issue/details/issuesList.tsx b/static/app/views/alerts/rules/issue/details/issuesList.tsx
--- a/static/app/views/alerts/rules/issue/details/issuesList.tsx
+++ b/static/app/views/alerts/rules/issue/details/issuesList.tsx
@@ -46,7 +46,7 @@
   cursor,
 }: Props) {
   const organization = useOrganization();
-  const {data, isPending, error} = useQuery({
+  const {data, isPending, isError, error} = useQuery({
     ...apiOptions.as<GroupHistory[]>()(
       '/projects/$organizationIdOrSlug/$projectIdOrSlug/rules/$ruleId/group-history/',
       {
@@ -70,10 +70,14 @@
   });
   const groupHistory = data?.json;
 
-  if (error instanceof RequestError) {
+  if (isError) {
     return (
       <LoadingError
-        message={(error.responseJSON?.detail as string) ?? t('default message')}
+        message={
+          error instanceof RequestError
+            ? ((error.responseJSON?.detail as string) ?? t('default message'))
+            : t('default message')
+        }
       />
     );
   }

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

Reviewed by Cursor Bugbot for commit e79ffcc. Configure here.

Comment thread static/app/views/alerts/rules/issue/details/issuesList.tsx
Copy link
Copy Markdown
Contributor

@nsdeschenes nsdeschenes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 🌔

@TkDodo TkDodo merged commit d4ad198 into master Apr 7, 2026
68 checks passed
@TkDodo TkDodo deleted the tkdodo/ref/header-queries-to-apiOptions branch April 7, 2026 13:22
@TkDodo TkDodo restored the tkdodo/ref/header-queries-to-apiOptions branch April 7, 2026 13:35
@TkDodo TkDodo deleted the tkdodo/ref/header-queries-to-apiOptions branch April 7, 2026 13:36
george-sentry pushed a commit that referenced this pull request Apr 9, 2026
…s to apiOptions (#112347)

as discussed in last week’s TSC, we need to start moving endpoints over
to `apiOptions` so that we can re-use caches.

the quickest win would be to implement `useApiQuery` with `apiOptions`
internally, and to get there, we need to migrate endpoints that now need
`getResponseHeader` over to `apiOptions` because those headers are
exposed differently.

this PR takes the first couple of endpoints that are (mostly)
self-contained (no other usages of the url found) and moves them over.

I’ve also exposed `selectJsonWithHeaders` because the default impl only
selects `json` without `headers`.

I plan to tackle the other occurrences with an endpoint-by-endpoint
approach, as we need to identify all places where an endpoint is used
(e.g. `invalidateQueries` or `getApiQueryData` or `setApiQueryData`) and
migrate them together.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants