ref(insights): Replace usage of <ReleaseSeries> in <InsightsTimeSeriesWidget>
#86300
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
Scope: Frontend
billyvg
changed the title
<ReleaseSeries> in <InsightsTimeSeriesWidget> (#86129)"<ReleaseSeries> in <InsightsTimeSeriesWidget>
billyvg
commented
Mar 4, 2025
Comment on lines
13
to
15
| datetime: Parameters<typeof normalizeDateTimeParams>[0]; | ||
| environments: readonly string[]; | ||
| projects: readonly number[]; |
There was a problem hiding this comment.
I decided to change this from using PageFilters because it was annoying to use this when you don't give it pagefilter selection params directly (e.g. issue details uses eventView). Instead, since we pass the datetime options to normalizeDateTimeParams, use its param type.
Comment on lines
17
to
21
| /** | ||
| * Max number of pages to fetch. Default is 10 pages, which should be | ||
| * sufficient to fetch "all" releases. | ||
| */ | ||
| maxPages?: number; |
There was a problem hiding this comment.
Also decided to add this so we can control the # of pages to fetch and to limit the damage if something were to go wrong or if an org has more releases than we expect. For reference, Sentry fetches ~3 pages for a 90d window.
| @@ -62,6 +62,7 @@ export type ApiQueryKey = | |||
| Record<string, any>, | |||
| Record<string, any> | |||
| >, | |||
| additionalKey?: string, | |||
There was a problem hiding this comment.
I think the better fix would be to have useInfiniteApiQuery always append an additional ’infinite’ key. This would avoid future conflicts if the same endpoint is used as query and infinite query. I think there’s only one other usage of useInfiniteApiQuery so it shouldn’t be a huge impact.
There was a problem hiding this comment.
I agree, though it gets kind of messy because queryKey can be 1 or 2 elements
I suppose we can check the queryKey length and do something like:
const query = useInfiniteQuery({
queryKey:
queryKey.length === 1
? ([...queryKey, {}, 'infinite'] as const)
: ([...queryKey, 'infinite'] as const),
There was a problem hiding this comment.
yeah that’s not too bad. What it would then take is to make an InfiniteApiQueryKey type that we would use in fetchInfiniteQuery. useInfiniteApiQuery would still accept an ApiQueryKey, and we would do the transformation internally.
I split up the types a bit to get there:
type UrlOnly = readonly [url: string];
type UrlWithOptions = readonly [
url: string,
options: QueryKeyEndpointOptions<
Record<string, string>,
Record<string, any>,
Record<string, any>
>,
];
export type ApiQueryKey = UrlOnly | UrlWithOptions;
type InfiniteApiQueryKey = readonly [...UrlWithOptions, {type: 'infinite'}];
There was a problem hiding this comment.
sgtm, will do this in a followup
Comment on lines
44
to
47
| const releases = | ||
| data?.pages.flatMap(([pageData]) => | ||
| pageData.map(({date, version}) => ({timestamp: date, version})) | ||
| ) ?? []; |
There was a problem hiding this comment.
Moved this mapping to where it's needed
billyvg
commented
Mar 4, 2025
TkDodo
reviewed
Mar 5, 2025
static/app/utils/useReleaseStats.tsx
Outdated
| 'load-all', | ||
| ], | ||
| ...queryOptions, | ||
| enabled: currentNumberPages.current < maxPages, |
There was a problem hiding this comment.
I don’t think disabling when the number of pages is reached is necessary - the effect will stop fetching anyways.
static/app/utils/useReleaseStats.tsx
Outdated
| }); | ||
|
|
||
| useEffect(() => { | ||
| if (!isFetching && hasNextPage && currentNumberPages.current + 1 < maxPages) { |
There was a problem hiding this comment.
tracking current pages in a ref is something you could also achieve by computing the number from the query result:
const currentNumberPages = data?.pages.length ?? 0
you can compute this outside of the useEffect, then set it as a dependency and use it inside.
| @@ -62,6 +62,7 @@ export type ApiQueryKey = | |||
| Record<string, any>, | |||
| Record<string, any> | |||
| >, | |||
| additionalKey?: string, | |||
There was a problem hiding this comment.
I think the better fix would be to have useInfiniteApiQuery always append an additional ’infinite’ key. This would avoid future conflicts if the same endpoint is used as query and infinite query. I think there’s only one other usage of useInfiniteApiQuery so it shouldn’t be a huge impact.
Bundle ReportChanges will increase total bundle size by 123.22kB (0.29%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: app-webpack-bundle-array-pushAssets Changed:
Files in
Files in
|
philipphofmann
pushed a commit
that referenced
this pull request
Mar 6, 2025
…riesWidget>` (#86300) This reverts commit 3febd2b and extends the [original PR](#86129) It was reverted because it was causing crashes (https://sentry.sentry.io/issues/6352301875/?project=11276&referrer=github-pr-bot) due to a cache key conflict in react-query between `useQuery` and `useInfiniteQuery` ([read more about it here](https://tkdodo.eu/blog/effective-react-query-keys#caching-data)). This would happen in Sentry if you visited an insights page (which uses an "infinite" query) and then went to e.g. the issue details page (which has it's own hook [useReleaseMarkLineSeries](https://github.com/getsentry/sentry/blob/master/static/app/views/issueDetails/streamline/hooks/useReleaseMarkLineSeries.tsx#L25-L45) that uses a "normal" query). Both queries use the same cache key because infinite queries handles the `cursor` param for us (thus it not being part of the query key). I've made some additional changes to the hook: it's now moved to `app/utils` as it'll be needed in other places. it's also more generic now. From the original PR: > This will be needed for the new release bubbles feature as we will show releases on the mini widgets instead of only on fullscreen. Using `<ReleaseSeries>` renderer will cause duplicate requests to the API. ref #85779
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reverts commit 3febd2b and extends the original PR
It was reverted because it was causing crashes (https://sentry.sentry.io/issues/6352301875/?project=11276&referrer=github-pr-bot) due to a cache key conflict in react-query between
useQueryanduseInfiniteQuery(read more about it here). This would happen in Sentry if you visited an insights page (which uses an "infinite" query) and then went to e.g. the issue details page (which has it's own hook useReleaseMarkLineSeries that uses a "normal" query). Both queries use the same cache key because infinite queries handles thecursorparam for us (thus it not being part of the query key).I've made some additional changes to the hook: it's now moved to
app/utilsas it'll be needed in other places. it's also more generic now.From the original PR:
ref #85779