diff --git a/static/app/components/charts/eventsChart.tsx b/static/app/components/charts/eventsChart.tsx index d4d772c51980..1fe1e5b2839c 100644 --- a/static/app/components/charts/eventsChart.tsx +++ b/static/app/components/charts/eventsChart.tsx @@ -21,7 +21,7 @@ import ChartZoom from 'sentry/components/charts/chartZoom'; import {ErrorPanel} from 'sentry/components/charts/errorPanel'; import type {LineChartProps} from 'sentry/components/charts/lineChart'; import {LineChart} from 'sentry/components/charts/lineChart'; -import ReleaseSeries from 'sentry/components/charts/releaseSeries'; +import {ReleaseSeries} from 'sentry/components/charts/releaseSeries'; import {TransitionChart} from 'sentry/components/charts/transitionChart'; import {TransparentLoadingMask} from 'sentry/components/charts/transparentLoadingMask'; import {getInterval, RELEASE_LINES_THRESHOLD} from 'sentry/components/charts/utils'; diff --git a/static/app/components/charts/releaseSeries.spec.tsx b/static/app/components/charts/releaseSeries.spec.tsx index 3e341b17f07f..249cfe40a662 100644 --- a/static/app/components/charts/releaseSeries.spec.tsx +++ b/static/app/components/charts/releaseSeries.spec.tsx @@ -1,13 +1,10 @@ import {Fragment} from 'react'; import {OrganizationFixture} from 'sentry-fixture/organization'; -import {ThemeFixture} from 'sentry-fixture/theme'; import {act, render, screen, waitFor} from 'sentry-test/reactTestingLibrary'; import type {ReleaseSeriesProps} from 'sentry/components/charts/releaseSeries'; -import ReleaseSeries from 'sentry/components/charts/releaseSeries'; - -const theme = ThemeFixture(); +import {ReleaseSeries} from 'sentry/components/charts/releaseSeries'; describe('ReleaseSeries', () => { const renderFunc = jest.fn(() => null); @@ -31,9 +28,7 @@ describe('ReleaseSeries', () => { }); }); - const baseSeriesProps: Omit = { - api: new MockApiClient(), - organization: OrganizationFixture(), + const baseSeriesProps: ReleaseSeriesProps = { period: '14d', start: null, end: null, @@ -42,12 +37,11 @@ describe('ReleaseSeries', () => { query: '', environments: [], children: renderFunc, - theme, }; it('does not fetch releases if releases is truthy', () => { render( - + {renderFunc} ); @@ -57,7 +51,7 @@ describe('ReleaseSeries', () => { it('does not fetch releases if not enabled', () => { render( - + {renderFunc} ); @@ -67,7 +61,7 @@ describe('ReleaseSeries', () => { it('fetches releases if becomes enabled', async () => { const {rerender} = render( - + {renderFunc} ); @@ -75,7 +69,7 @@ describe('ReleaseSeries', () => { expect(releasesMock).not.toHaveBeenCalled(); rerender( - + {renderFunc} ); @@ -85,7 +79,7 @@ describe('ReleaseSeries', () => { expect(releasesMock).toHaveBeenCalledTimes(1); rerender( - + {renderFunc} ); diff --git a/static/app/components/charts/releaseSeries.tsx b/static/app/components/charts/releaseSeries.tsx index 10aeadc54cb4..5af5e1d1b2ad 100644 --- a/static/app/components/charts/releaseSeries.tsx +++ b/static/app/components/charts/releaseSeries.tsx @@ -1,8 +1,6 @@ -import {Component} from 'react'; -import type {Theme} from '@emotion/react'; -import {withTheme} from '@emotion/react'; -import type {Location, Query} from 'history'; -import isEqual from 'lodash/isEqual'; +import {useEffect, useRef, useState} from 'react'; +import {useTheme} from '@emotion/react'; +import type {Query} from 'history'; import memoize from 'lodash/memoize'; import partition from 'lodash/partition'; @@ -18,12 +16,11 @@ import {escape} from 'sentry/utils'; import {getApiUrl} from 'sentry/utils/api/getApiUrl'; import {getFormat, getFormattedDate, getUtcDateString} from 'sentry/utils/dates'; import {parseLinkHeader} from 'sentry/utils/parseLinkHeader'; +import {useApi} from 'sentry/utils/useApi'; import {useLocation} from 'sentry/utils/useLocation'; -import type {ReactRouter3Navigate} from 'sentry/utils/useNavigate'; import {useNavigate} from 'sentry/utils/useNavigate'; +import {useOrganization} from 'sentry/utils/useOrganization'; import {formatVersion} from 'sentry/utils/versions/formatVersion'; -import {withApi} from 'sentry/utils/withApi'; -import {withOrganization} from 'sentry/utils/withOrganization'; import {makeReleasesPathname} from 'sentry/views/explore/releases/utils/pathnames'; type ReleaseMetaBasic = { @@ -80,16 +77,11 @@ const getOrganizationReleasesMemoized = memoize( ); export interface ReleaseSeriesProps { - api: Client; - children: (s: State) => React.ReactNode; + children: (s: ReleaseSeriesState) => React.ReactNode; end: DateString; environments: readonly string[]; - location: Location; - navigate: ReactRouter3Navigate; - organization: Organization; projects: readonly number[]; start: DateString; - theme: Theme; emphasizeReleases?: string[]; enabled?: boolean; memoized?: boolean; @@ -102,251 +94,291 @@ export interface ReleaseSeriesProps { utc?: boolean | null; } -type State = { +type ReleaseSeriesState = { releaseSeries: Series[]; releases: ReleaseMetaBasic[] | null; }; +type UseReleaseSeriesProps = Omit; + /** * @deprecated use useReleaseBubbles instead */ -class ReleaseSeries extends Component { - state: State = { +export function useReleaseSeries({ + start, + end, + period, + environments, + projects, + query, + releases: propReleases, + enabled = true, + memoized, + emphasizeReleases, + preserveQueryParams, + queryExtra, + tooltip, + utc, +}: UseReleaseSeriesProps): ReleaseSeriesState { + const api = useApi(); + const organization = useOrganization(); + const theme = useTheme(); + const location = useLocation(); + const navigate = useNavigate(); + + // Tooltip formatters live inside echarts and are not re-created when utc changes + const utcRef = useRef(utc); + useEffect(() => { + utcRef.current = utc; + }, [utc]); + + const [state, setState] = useState({ releases: null, releaseSeries: [], - }; + }); - componentDidMount() { - this._isMounted = true; - const {releases, enabled = true} = this.props; + // Callers like Discover rebuild Date objects and arrays on every render, so + // effect deps compare serialized values to avoid re-fetching unchanged data. + const startKey = start ? getUtcDateString(start) : ''; + const endKey = end ? getUtcDateString(end) : ''; + const projectsKey = [...projects].join(','); + const environmentsKey = [...environments].join(','); + const emphasizeReleasesKey = emphasizeReleases?.join(',') ?? ''; - if (releases) { - // No need to fetch releases if passed in from props - this.setReleasesWithSeries(releases); - return; - } + // Stable refs for values used inside closures passed to echarts + const organizationRef = useRef(organization); + const locationRef = useRef(location); + const navigateRef = useRef(navigate); + const themeRef = useRef(theme); + const preserveQueryParamsRef = useRef(preserveQueryParams); + const queryExtraRef = useRef(queryExtra); + const tooltipRef = useRef(tooltip); + const environmentsRef = useRef(environments); + const startRef = useRef(start); + const endRef = useRef(end); + const periodRef = useRef(period); - if (enabled) { - this.fetchData(); - } - } + useEffect(() => { + organizationRef.current = organization; + locationRef.current = location; + navigateRef.current = navigate; + themeRef.current = theme; + preserveQueryParamsRef.current = preserveQueryParams; + queryExtraRef.current = queryExtra; + tooltipRef.current = tooltip; + environmentsRef.current = environments; + startRef.current = start; + endRef.current = end; + periodRef.current = period; + }); - componentDidUpdate(prevProps: any) { - const {enabled = true} = this.props; - - if ( - (!isEqual(prevProps.projects, this.props.projects) || - !isEqual(prevProps.environments, this.props.environments) || - !isEqual(prevProps.start, this.props.start) || - !isEqual(prevProps.end, this.props.end) || - !isEqual(prevProps.period, this.props.period) || - !isEqual(prevProps.query, this.props.query) || - (!prevProps.enabled && this.props.enabled)) && - enabled - ) { - this.fetchData(); - } else if (!isEqual(prevProps.emphasizeReleases, this.props.emphasizeReleases)) { - this.setReleasesWithSeries(this.state.releases); - } - } + function buildReleaseSeries(releases: ReleaseMetaBasic[]): Series[] { + const releaseSeries: Series[] = []; - componentWillUnmount() { - this._isMounted = false; - this.props.api.clear(); - } + function makeOneSeries(items: ReleaseMetaBasic[], lineStyle = {}): Series { + const releaseColor = themeRef.current.tokens.dataviz.semantic.release; - _isMounted = false; - - async fetchData() { - const { - api, - organization, - projects, - environments, - period, - start, - end, - memoized, - query, - } = this.props; - const conditions: ReleaseConditions = { - start, - end, - project: projects, - environment: environments, - statsPeriod: period, - query, - }; - let hasMore = true; - const releases: ReleaseMetaBasic[] = []; - while (hasMore) { - try { - const getReleases = memoized - ? getOrganizationReleasesMemoized - : getOrganizationReleases; - const [newReleases, , resp] = await getReleases(api, organization, conditions); - releases.push(...newReleases); - if (this._isMounted) { - this.setReleasesWithSeries(releases); - } - - const pageLinks = resp?.getResponseHeader('Link'); - if (pageLinks) { - const paginationObject = parseLinkHeader(pageLinks); - hasMore = paginationObject?.next?.results ?? false; - conditions.cursor = paginationObject.next!.cursor; - } else { - hasMore = false; - } - } catch { - addErrorMessage(t('Error fetching releases')); - hasMore = false; + const extraQuery: Query = {...queryExtraRef.current}; + extraQuery.project = locationRef.current.query.project; + if (preserveQueryParamsRef.current) { + extraQuery.environment = [...environmentsRef.current]; + extraQuery.start = startRef.current + ? getUtcDateString(startRef.current) + : undefined; + extraQuery.end = endRef.current ? getUtcDateString(endRef.current) : undefined; + extraQuery.statsPeriod = periodRef.current || undefined; } - } - } - setReleasesWithSeries(releases: any) { - const {emphasizeReleases = []} = this.props; - const releaseSeries: Series[] = []; + const markLine = createMarkLine({ + animation: false, + lineStyle: { + color: releaseColor, + opacity: 0.3, + type: 'solid', + ...lineStyle, + }, + label: { + show: false, + }, + data: items.map((release: ReleaseMetaBasic) => ({ + xAxis: +new Date(release.date), + name: formatVersion(release.version, true), + value: formatVersion(release.version, true), - if (emphasizeReleases.length) { + onClick: () => { + navigateRef.current({ + pathname: makeReleasesPathname({ + organization: organizationRef.current, + path: `/${encodeURIComponent(release.version)}/`, + }), + query: extraQuery, + }); + }, + + label: { + formatter: () => formatVersion(release.version, true), + }, + })), + tooltip: tooltipRef.current || { + trigger: 'item', + formatter: ({data}: any) => { + // Should only happen when navigating pages + if (!data) { + return ''; + } + const time = getFormattedDate( + data.value, + getFormat({timeZone: true, year: true}), + { + local: !utcRef.current, + } + ); + const version = escape(formatVersion(data.name, true)); + return [ + '
', + `
${t( + 'Release' + )} ${version}
`, + '
', + '', + '
', + ].join(''); + }, + }, + }); + + return { + id: 'release-lines', + seriesName: 'Releases', + color: releaseColor, + data: [], + markLine, + }; + } + + if (emphasizeReleases?.length) { const [unemphasizedReleases, emphasizedReleases] = partition( releases, - release => !emphasizeReleases.includes(release.version) + release => !emphasizeReleases!.includes(release.version) ); if (unemphasizedReleases.length) { - releaseSeries.push(this.getReleaseSeries(unemphasizedReleases, {type: 'dotted'})); + releaseSeries.push(makeOneSeries(unemphasizedReleases, {type: 'dotted'})); } if (emphasizedReleases.length) { releaseSeries.push( - this.getReleaseSeries(emphasizedReleases, { + makeOneSeries(emphasizedReleases, { opacity: 0.8, }) ); } } else { - releaseSeries.push(this.getReleaseSeries(releases)); + releaseSeries.push(makeOneSeries(releases)); } - this.setState({ - releases, - releaseSeries, - }); + return releaseSeries; } - getReleaseSeries = (releases: any, lineStyle = {}) => { - const { - organization, - location, - navigate, - tooltip, - environments, - start, - end, - period, - preserveQueryParams, - queryExtra, - theme, - } = this.props; - - const query = {...queryExtra}; - query.project = location.query.project; - if (preserveQueryParams) { - query.environment = [...environments]; - query.start = start ? getUtcDateString(start) : undefined; - query.end = end ? getUtcDateString(end) : undefined; - query.statsPeriod = period || undefined; + useEffect(() => { + if (propReleases) { + setState({ + releases: propReleases, + releaseSeries: buildReleaseSeries(propReleases), + }); + return; } - const markLine = createMarkLine({ - animation: false, - lineStyle: { - color: theme.tokens.dataviz.semantic.release, - opacity: 0.3, - type: 'solid', - ...lineStyle, - }, - label: { - show: false, - }, - data: releases.map((release: any) => ({ - xAxis: +new Date(release.date), - name: formatVersion(release.version, true), - value: formatVersion(release.version, true), - - onClick: () => { - navigate({ - pathname: makeReleasesPathname({ - organization, - path: `/${encodeURIComponent(release.version)}/`, - }), - query, - }); - }, + if (!enabled) { + return; + } - label: { - formatter: () => formatVersion(release.version, true), - }, - })), - tooltip: tooltip || { - trigger: 'item', - formatter: ({data}: any) => { - // Should only happen when navigating pages - if (!data) { - return ''; + let cancelled = false; + + async function fetchData() { + const conditions: ReleaseConditions = { + start, + end, + project: projects, + environment: environments, + statsPeriod: period, + query, + }; + + let hasMore = true; + const releases: ReleaseMetaBasic[] = []; + while (hasMore) { + try { + const getReleases = memoized + ? getOrganizationReleasesMemoized + : getOrganizationReleases; + const [newReleases, , resp] = await getReleases(api, organization, conditions); + releases.push(...newReleases); + if (!cancelled) { + setState({ + releases, + releaseSeries: buildReleaseSeries(releases), + }); } - // XXX using this.props here as this function does not get re-run - // unless projects are changed. Using a closure variable would result - // in stale values. - const time = getFormattedDate( - data.value, - getFormat({timeZone: true, year: true}), - { - local: !this.props.utc, - } - ); - const version = escape(formatVersion(data.name, true)); - return [ - '
', - `
${t( - 'Release' - )} ${version}
`, - '
', - '', - '
', - ].join(''); - }, - }, - }); - return { - id: 'release-lines', - seriesName: 'Releases', - color: theme.tokens.dataviz.semantic.release, - data: [], - markLine, - }; - }; + const pageLinks = resp?.getResponseHeader('Link'); + if (pageLinks) { + const paginationObject = parseLinkHeader(pageLinks); + hasMore = paginationObject?.next?.results ?? false; + conditions.cursor = paginationObject.next!.cursor; + } else { + hasMore = false; + } + } catch { + addErrorMessage(t('Error fetching releases')); + hasMore = false; + } + } + } - render() { - const {children, enabled = true} = this.props; + fetchData(); + + return () => { + cancelled = true; + api.clear(); + }; + // eslint-disable-next-line react-hooks/exhaustive-deps -- serialized keys stand in for start/end/projects/environments + }, [ + startKey, + endKey, + period, + projectsKey, + environmentsKey, + query, + propReleases, + enabled, + memoized, + ]); - return children({ - releases: enabled ? this.state.releases : [], - releaseSeries: enabled ? this.state.releaseSeries : [], + // Rebuild series when emphasizeReleases changes without re-fetching. + useEffect(() => { + setState(prev => { + if (prev.releases === null) { + return prev; + } + return { + ...prev, + releaseSeries: buildReleaseSeries(prev.releases), + }; }); + // eslint-disable-next-line react-hooks/exhaustive-deps -- serialized key stands in for emphasizeReleases + }, [emphasizeReleasesKey]); + + if (!enabled) { + return {releases: [], releaseSeries: []}; } -} -function WithRouter(props: Omit) { - const location = useLocation(); - const navigate = useNavigate(); - return ; + return state; } /** - * @deprecated use useReleaseBubbles instead + * @deprecated use useReleaseSeries hook instead */ -export default withOrganization(withApi(withTheme(WithRouter))); +export function ReleaseSeries({children, ...props}: ReleaseSeriesProps) { + return children(useReleaseSeries(props)); +} diff --git a/static/app/views/insights/common/components/chart.tsx b/static/app/views/insights/common/components/chart.tsx index 23ef214e8da8..6f35920082fd 100644 --- a/static/app/views/insights/common/components/chart.tsx +++ b/static/app/views/insights/common/components/chart.tsx @@ -19,7 +19,7 @@ import ChartZoom, {type ZoomRenderProps} from 'sentry/components/charts/chartZoo import type {FormatterOptions} from 'sentry/components/charts/components/tooltip'; import {getFormatter} from 'sentry/components/charts/components/tooltip'; import {ErrorPanel} from 'sentry/components/charts/errorPanel'; -import ReleaseSeries from 'sentry/components/charts/releaseSeries'; +import {useReleaseSeries} from 'sentry/components/charts/releaseSeries'; import {createLineSeries} from 'sentry/components/charts/series/lineSeries'; import {TransitionChart} from 'sentry/components/charts/transitionChart'; import {TransparentLoadingMask} from 'sentry/components/charts/transparentLoadingMask'; @@ -244,6 +244,17 @@ export function Chart({ const height = renderingContext?.height ?? chartHeight; const isLegendVisible = renderingContext?.isFullscreen ?? showLegend; + const {releaseSeries} = useReleaseSeries({ + start, + end, + queryExtra: undefined, + period, + utc, + projects, + environments, + enabled: renderingContext?.isFullscreen ?? false, + }); + const defaultRef = useRef(null); const chartRef = ref || defaultRef; @@ -472,23 +483,11 @@ export function Chart({ {zoomRenderProps => renderingContext?.isFullscreen ? ( - - {({releaseSeries}) => ( - - )} - + ) : ( ) diff --git a/static/app/views/projectDetail/charts/projectBaseSessionsChart.tsx b/static/app/views/projectDetail/charts/projectBaseSessionsChart.tsx index 020e4c143db3..d41f448939a8 100644 --- a/static/app/views/projectDetail/charts/projectBaseSessionsChart.tsx +++ b/static/app/views/projectDetail/charts/projectBaseSessionsChart.tsx @@ -13,7 +13,7 @@ import ChartZoom from 'sentry/components/charts/chartZoom'; import {ErrorPanel} from 'sentry/components/charts/errorPanel'; import type {LineChartProps} from 'sentry/components/charts/lineChart'; import {LineChart} from 'sentry/components/charts/lineChart'; -import ReleaseSeries from 'sentry/components/charts/releaseSeries'; +import {useReleaseSeries} from 'sentry/components/charts/releaseSeries'; import {StackedAreaChart} from 'sentry/components/charts/stackedAreaChart'; import {HeaderTitleLegend} from 'sentry/components/charts/styles'; import {TransitionChart} from 'sentry/components/charts/transitionChart'; @@ -67,6 +67,16 @@ function ProjectBaseSessionsChart({ const {projects, environments, datetime} = selection; const {start, end, period, utc} = datetime; + const {releaseSeries} = useReleaseSeries({ + utc, + period, + start, + end, + projects, + environments, + query, + }); + const Request = [DisplayModes.ANR_RATE, DisplayModes.FOREGROUND_ANR_RATE].includes( displayMode ) @@ -95,53 +105,39 @@ function ProjectBaseSessionsChart({ timeseriesData, previousTimeseriesData, additionalSeries, - }) => ( - - {({releaseSeries}) => { - if (errored) { - return ( - - - - ); - } - - return ( - - - - - {title} - {help && } - - - - - ); - }} - - )} + }) => { + if (errored) { + return ( + + + + ); + } + + return ( + + + + + {title} + {help && } + + + + + ); + }} )}