Skip to content

feat(releases): Refactor to support deep-linked Issues Chart #89711

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 24, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion static/app/components/charts/chartWidgetLoader.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {useQuery} from '@tanstack/react-query';
import Placeholder from 'sentry/components/placeholder';
import {t} from 'sentry/locale';
import type {LoadableChartWidgetProps} from 'sentry/views/insights/common/components/widgets/types';
import {EVENT_GRAPH_WIDGET_ID} from 'sentry/views/issueDetails/streamline/eventGraphWidget';

interface Props extends LoadableChartWidgetProps {
/**
Expand All @@ -26,7 +27,13 @@ interface Props extends LoadableChartWidgetProps {
export function ChartWidgetLoader(props: Props) {
const query = useQuery<{default: React.FC<LoadableChartWidgetProps>}>({
queryKey: [`widget-${props.id}`],
queryFn: () => import(`sentry/views/insights/common/components/widgets/${props.id}`),
queryFn: () => {
if (props.id === EVENT_GRAPH_WIDGET_ID) {
Copy link
Member

Choose a reason for hiding this comment

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

does this lead to circular dependencies?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good thinking... it could if we used ChartWidgetLoader to always load EventGraph, but we shouldn't need to. It'll only be the Insights widgets that we replace with WidgetLoader

return import('sentry/views/issueDetails/streamline/eventGraphWidget');
}

return import(`sentry/views/insights/common/components/widgets/${props.id}`);
},
});

if (query.isPending) {
Expand Down
42 changes: 27 additions & 15 deletions static/app/views/issueDetails/streamline/eventGraph.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import type {ReactEchartsRef, SeriesDataUnit} from 'sentry/types/echarts';
import type {Event} from 'sentry/types/event';
import type {Group} from 'sentry/types/group';
import type {EventsStats, MultiSeriesEventsStats} from 'sentry/types/organization';
import type EventView from 'sentry/utils/discover/eventView';
import {DiscoverDatasets} from 'sentry/utils/discover/types';
import {formatAbbreviatedNumber} from 'sentry/utils/formatters';
import {getConfigForIssueType} from 'sentry/utils/issueTypeConfig';
Expand All @@ -48,7 +49,7 @@ import {Tab} from 'sentry/views/issueDetails/types';
import {useGroupDetailsRoute} from 'sentry/views/issueDetails/useGroupDetailsRoute';
import {useReleaseBubbles} from 'sentry/views/releases/releaseBubbles/useReleaseBubbles';

const enum EventGraphSeries {
enum EventGraphSeries {
EVENT = 'event',
USER = 'user',
}
Expand All @@ -64,6 +65,7 @@ interface EventGraphProps {
* chart).
*/
disableZoomNavigation?: boolean;
eventView?: EventView;
ref?: React.Ref<ReactEchartsRef>;
/**
* Configures showing releases on the chart as bubbles or lines. This is used
Expand Down Expand Up @@ -100,6 +102,7 @@ function createSeriesAndCount(stats: EventsStats) {
export function EventGraph({
group,
event,
eventView: eventViewProps,
disableZoomNavigation = false,
showReleasesAs,
showSummary = true,
Expand Down Expand Up @@ -131,7 +134,8 @@ export function EventGraph({
ref: chartContainerRef,
onResize,
});
const eventView = useIssueDetailsEventView({group, isSmallContainer});
const eventViewHook = useIssueDetailsEventView({group, isSmallContainer});
const eventView = eventViewProps || eventViewHook;

const {
data: groupStats = {},
Expand Down Expand Up @@ -447,18 +451,22 @@ export function EventGraph({
if (isLoadingStats || isPendingUniqueUsersCount) {
return (
<GraphWrapper {...styleProps}>
<SummaryContainer>
<GraphButton
isActive={visibleSeries === EventGraphSeries.EVENT}
disabled
label={t('Events')}
/>
<GraphButton
isActive={visibleSeries === EventGraphSeries.USER}
disabled
label={t('Users')}
/>
</SummaryContainer>
{showSummary ? (
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes a bug in Release drawer where this is shown during loading state.

<SummaryContainer>
<GraphButton
isActive={visibleSeries === EventGraphSeries.EVENT}
disabled
label={t('Events')}
/>
<GraphButton
isActive={visibleSeries === EventGraphSeries.USER}
disabled
label={t('Users')}
/>
</SummaryContainer>
) : (
<div />
Copy link
Member

Choose a reason for hiding this comment

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

could this be null or undefined instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Container is a grid so it needs an empty el unfortunately :(

)}
<LoadingChartContainer ref={chartContainerRef}>
<Placeholder height="96px" testId="event-graph-loading" />
</LoadingChartContainer>
Expand Down Expand Up @@ -561,7 +569,11 @@ function GraphButton({
label,
count,
...props
}: {isActive: boolean; label: string; count?: string} & Partial<ButtonProps>) {
}: {
isActive: boolean;
label: string;
count?: string;
} & Partial<ButtonProps>) {
return (
<CalloutButton
isActive={isActive}
Expand Down
58 changes: 58 additions & 0 deletions static/app/views/issueDetails/streamline/eventGraphWidget.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import styled from '@emotion/styled';

import Placeholder from 'sentry/components/placeholder';
import {t} from 'sentry/locale';
import {useParams} from 'sentry/utils/useParams';
import type {LoadableChartWidgetProps} from 'sentry/views/insights/common/components/widgets/types';
import {EventGraph} from 'sentry/views/issueDetails/streamline/eventGraph';
import {useIssueDetailsEventView} from 'sentry/views/issueDetails/streamline/hooks/useIssueDetailsDiscoverQuery';
import {useGroup} from 'sentry/views/issueDetails/useGroup';

export default function EventGraphWidget({pageFilters}: LoadableChartWidgetProps) {
const {groupId} = useParams();

const {
data: groupData,
isPending: loadingGroup,
isError: isGroupError,
} = useGroup({groupId: groupId!});

const eventView = useIssueDetailsEventView({
group: groupData!,
isSmallContainer: true,
pageFilters,
});

if (loadingGroup) {
return (
<Container>
<Placeholder height="100%" />
</Container>
);
}

if (isGroupError) {
return (
<Container>
<Placeholder height="100%" error={t('Error loading chart')} />
</Container>
);
}

return (
<EventGraph
event={undefined}
eventView={eventView}
group={groupData}
showSummary={false}
showReleasesAs="line"
disableZoomNavigation
/>
);
}

const Container = styled('div')`
height: 100%;
`;

export const EVENT_GRAPH_WIDGET_ID = 'event-graph-widget';
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {getInterval} from 'sentry/components/charts/utils';
import type {PageFilters} from 'sentry/types/core';
import type {Group} from 'sentry/types/group';
import type {NewQuery, SavedQuery} from 'sentry/types/organization';
import EventView from 'sentry/utils/discover/eventView';
Expand All @@ -16,11 +17,13 @@ import {useGroupDefaultStatsPeriod} from 'sentry/views/issueDetails/useGroupDefa

export function useIssueDetailsEventView({
group,
pageFilters,
queryProps,
isSmallContainer = false,
}: {
group: Group;
isSmallContainer?: boolean;
pageFilters?: PageFilters;
queryProps?: Partial<SavedQuery>;
}) {
const searchQuery = useEventQuery({groupId: group.id});
Expand All @@ -29,13 +32,15 @@ export function useIssueDetailsEventView({
const hasSetStatsPeriod =
location.query.statsPeriod || location.query.start || location.query.end;
const defaultStatsPeriod = useGroupDefaultStatsPeriod(group, group.project);
const periodQuery = hasSetStatsPeriod
? getPeriod({
start: location.query.start as string,
end: location.query.end as string,
period: location.query.statsPeriod as string,
})
: defaultStatsPeriod;
const periodQuery = pageFilters
? getPeriod(pageFilters.datetime)
: hasSetStatsPeriod
? getPeriod({
start: location.query.start as string,
end: location.query.end as string,
period: location.query.statsPeriod as string,
})
: defaultStatsPeriod;

const interval = getInterval(
{
Expand Down
Loading