Skip to content

Commit

Permalink
ref(feedback): fix show/hide logic for trace section, use placeholder…
Browse files Browse the repository at this point in the history
… while loading (#79284)

Ref #68481

The show/hide logic had a bug where we were showing only if the
same-trace issue is a duplicate of a crash report linked error. Which is
the opposite of what we want. Example:
[JAVASCRIPT-2VE1](https://sentry.sentry.io/feedback/?feedbackSlug=javascript%3A5735318217&project=11276).
_Analytics are still correct, but atm no timelines (2+ events) are
shown_.

Also: 
- use placeholder when the data is fetching.
- use error boundary.
- remove the `hasProject` prop. It makes the logic hard to read, and was
only used for the crash-report-dup condition. Comparing the issue ids is
sufficient for this.
  • Loading branch information
aliu39 authored Oct 17, 2024
1 parent 78948de commit 17dc703
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 18 deletions.
8 changes: 3 additions & 5 deletions static/app/components/feedback/feedbackItem/feedbackItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,9 @@ export default function FeedbackItem({feedbackItem, eventData, tags}: Props) {
/>

{eventData ? (
<TraceDataSection
eventData={eventData}
crashReportId={crashReportId}
hasProject={!!feedbackItem.project}
/>
<ErrorBoundary mini>
<TraceDataSection eventData={eventData} crashReportId={crashReportId} />
</ErrorBoundary>
) : null}

<Section icon={<IconTag size="xs" />} title={t('Tags')}>
Expand Down
37 changes: 24 additions & 13 deletions static/app/components/feedback/feedbackItem/traceDataSection.tsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
import {useEffect} from 'react';

import Section from 'sentry/components/feedback/feedbackItem/feedbackItemSection';
import Placeholder from 'sentry/components/placeholder';
import {IconSpan} from 'sentry/icons';
import {t} from 'sentry/locale';
import type {Event} from 'sentry/types/event';
import {trackAnalytics} from 'sentry/utils/analytics';
import useOrganization from 'sentry/utils/useOrganization';
import {TraceDataSection as IssuesTraceDataSection} from 'sentry/views/issueDetails/traceDataSection';
import {useTraceTimelineEvents} from 'sentry/views/issueDetails/traceTimeline/useTraceTimelineEvents';
import {
type TimelineEvent,
useTraceTimelineEvents,
} from 'sentry/views/issueDetails/traceTimeline/useTraceTimelineEvents';

/**
* Doesn't require a Section wrapper. Rendered conditionally if
Expand All @@ -17,23 +21,17 @@ import {useTraceTimelineEvents} from 'sentry/views/issueDetails/traceTimeline/us
export default function TraceDataSection({
eventData,
crashReportId,
hasProject,
}: {
crashReportId: string | undefined;
eventData: Event;
hasProject: boolean;
}) {
// If there's a linked error from a crash report and only one other issue, showing both could be redundant.
// TODO: we could add a jest test .spec for this ^
const organization = useOrganization();
const {oneOtherIssueEvent, traceEvents, isLoading, isError} = useTraceTimelineEvents({
event: eventData,
});
const show =
!isLoading &&
!isError &&
traceEvents.length > 1 && // traceEvents include the current event.
(!hasProject || !crashReportId || oneOtherIssueEvent?.id === crashReportId);
// Note traceEvents includes the current event (feedback).

useEffect(() => {
if (isError) {
Expand All @@ -45,23 +43,36 @@ export default function TraceDataSection({
organization,
});
}
if (hasProject && !!crashReportId && oneOtherIssueEvent?.id === crashReportId) {
if (eventIsCrashReportDup(oneOtherIssueEvent, crashReportId)) {
trackAnalytics('feedback.trace-section.crash-report-dup', {organization});
}
}
}, [
crashReportId,
hasProject,
isError,
isLoading,
oneOtherIssueEvent?.id,
oneOtherIssueEvent,
organization,
traceEvents.length,
]);

return show && organization.features.includes('user-feedback-trace-section') ? (
return organization.features.includes('user-feedback-trace-section') &&
!isError &&
traceEvents.length > 1 &&
!eventIsCrashReportDup(oneOtherIssueEvent, crashReportId) ? (
<Section icon={<IconSpan size="xs" />} title={t('Data From The Same Trace')}>
<IssuesTraceDataSection event={eventData} />
{isLoading ? (
<Placeholder height="114px" />
) : (
<IssuesTraceDataSection event={eventData} />
)}
</Section>
) : null;
}

function eventIsCrashReportDup(
event: TimelineEvent | undefined,
crashReportId: string | undefined
) {
return !!crashReportId && event?.id === crashReportId;
}

0 comments on commit 17dc703

Please sign in to comment.