Skip to content
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
61 changes: 0 additions & 61 deletions static/app/components/assistant/getGuidesContent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,67 +10,6 @@ export function getGuidesContent(): GuidesContent {
return getDemoModeGuides();
}
return [
{
guide: 'issue',
requiredTargets: ['issue_header_stats', 'breadcrumbs', 'issue_sidebar_owners'],
steps: [
{
title: t('How bad is it?'),
target: 'issue_header_stats',
description: t(
`You have Issues and that's fine.
Understand impact at a glance by viewing total issue frequency and affected users.`
),
},
{
title: t('Find problematic releases'),
target: 'issue_sidebar_releases',
description: t(
'See which release introduced the issue and which release it last appeared in.'
),
},
{
title: t('Not your typical stack trace'),
target: 'stacktrace',
description: t(
`Sentry can show your source code in the stack trace.
See the exact sequence of function calls leading to the error in question.`
),
},
{
// TODO(streamline-ui): Remove from guides on GA, tag sidebar is gone
title: t('Pinpoint hotspots'),
target: 'issue_sidebar_tags',
description: t(
'Tags are key/value string pairs that are automatically indexed and searchable in Sentry.'
),
},
{
title: t('Retrace Your Steps'),
target: 'breadcrumbs',
description: t(
`Not sure how you got here? Sentry automatically captures breadcrumbs for
events your user and app took that led to the error.`
),
},
{
title: t('Annoy the Right People'),
target: 'issue_sidebar_owners',
description: t(
`Automatically assign issues to the person who introduced the commit,
notify them over notification tools like Slack,
and triage through issue management tools like Jira. `
),
},
{
title: t('Onboarding'),
target: 'onboarding_sidebar',
description: t(
'Walk through this guide to get the most out of Sentry right away.'
),
},
],
},
{
guide: 'issue_stream',
requiredTargets: ['issue_stream'],
Expand Down
38 changes: 17 additions & 21 deletions static/app/components/assistant/guideAnchor.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@ import {GuideStore} from 'sentry/stores/guideStore';
describe('GuideAnchor', () => {
const serverGuide = [
{
guide: 'issue',
guide: 'trace_view',
seen: false,
},
];
const firstGuideHeader = 'How bad is it?';
const firstGuideHeader = 'Event Breakdown';

beforeEach(() => {
ConfigStore.loadInitialData(
Expand All @@ -30,29 +30,25 @@ describe('GuideAnchor', () => {
it('renders, async advances, async and finishes', async () => {
render(
<div>
<GuideAnchor target="issue_header_stats" />
<GuideAnchor target="breadcrumbs" />
<GuideAnchor target="issue_sidebar_owners" />
<GuideAnchor target="trace_view_guide_breakdown" />
<GuideAnchor target="trace_view_guide_row" />
<GuideAnchor target="trace_view_guide_row_details" />
</div>
);

act(() => GuideStore.fetchSucceeded(serverGuide));
expect(await screen.findByText(firstGuideHeader)).toBeInTheDocument();

// XXX(epurkhiser): Skip pointer event checks due to a bug with how Popper
// renders the hovercard with pointer-events: none. See [0]
//
// [0]: https://github.com/testing-library/user-event/issues/639
//
// NOTE(epurkhiser): We may be able to remove the skipPointerEventsCheck
// when we're on popper >= 1.
await userEvent.click(screen.getByLabelText('Next'));

expect(await screen.findByText('Retrace Your Steps')).toBeInTheDocument();
expect(await screen.findByText('Events')).toBeInTheDocument();
expect(screen.queryByText(firstGuideHeader)).not.toBeInTheDocument();

await userEvent.click(screen.getByLabelText('Next'));

expect(await screen.findByText('Event Details')).toBeInTheDocument();
expect(screen.queryByText('Events')).not.toBeInTheDocument();

// Clicking on the button in the last step should finish the guide.
const finishMock = MockApiClient.addMockResponse({
method: 'PUT',
Expand All @@ -66,7 +62,7 @@ describe('GuideAnchor', () => {
expect.objectContaining({
method: 'PUT',
data: {
guide: 'issue',
guide: 'trace_view',
status: 'viewed',
},
})
Expand All @@ -76,9 +72,9 @@ describe('GuideAnchor', () => {
it('dismisses', async () => {
render(
<div>
<GuideAnchor target="issue_header_stats" />
<GuideAnchor target="breadcrumbs" />
<GuideAnchor target="issue_sidebar_owners" />
<GuideAnchor target="trace_view_guide_breakdown" />
<GuideAnchor target="trace_view_guide_row" />
<GuideAnchor target="trace_view_guide_row_details" />
</div>
);

Expand All @@ -97,7 +93,7 @@ describe('GuideAnchor', () => {
expect.objectContaining({
method: 'PUT',
data: {
guide: 'issue',
guide: 'trace_view',
status: 'dismissed',
},
})
Expand Down Expand Up @@ -131,9 +127,9 @@ describe('GuideAnchor', () => {
it('if forceHide is true, async do not render guide', async () => {
render(
<div>
<GuideAnchor target="issue_header_stats" />
<GuideAnchor target="breadcrumbs" />
<GuideAnchor target="issue_sidebar_owners" />
<GuideAnchor target="trace_view_guide_breakdown" />
<GuideAnchor target="trace_view_guide_row" />
<GuideAnchor target="trace_view_guide_row_details" />
</div>
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import {useMemo, useState} from 'react';
import {Grid} from '@sentry/scraps/layout';
import {SegmentedControl} from '@sentry/scraps/segmentedControl';

import {GuideAnchor} from 'sentry/components/assistant/guideAnchor';
import {EventTags} from 'sentry/components/events/eventTags';
import {
associateTagsWithMeta,
Expand Down Expand Up @@ -83,11 +82,7 @@ export function EventTagsDataSection({
return (
<FoldSection
disableCollapsePersistence={disableCollapsePersistence}
title={
<GuideAnchor target="tags" position="top">
{t('Tags')}
</GuideAnchor>
}
title={t('Tags')}
actions={actions}
sectionKey={SectionKey.TAGS}
ref={ref}
Expand Down
27 changes: 12 additions & 15 deletions static/app/stores/guideStore.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,14 @@ describe('GuideStore', () => {
GuideStore.init();
data = [
{
guide: 'issue',
guide: 'trace_view',
seen: false,
},
{guide: 'issue_stream', seen: true},
];
GuideStore.registerAnchor('issue_header_stats');
GuideStore.registerAnchor('issue_sidebar_owners');
GuideStore.registerAnchor('breadcrumbs');
GuideStore.registerAnchor('issue_stream');
GuideStore.registerAnchor('trace_view_guide_row');
GuideStore.registerAnchor('trace_view_guide_row_details');
});

afterEach(() => {
Expand All @@ -44,33 +43,31 @@ describe('GuideStore', () => {
GuideStore.fetchSucceeded(data);
// Should pick the first non-seen guide in alphabetic order.
expect(GuideStore.getState().currentStep).toBe(0);
expect(GuideStore.getState().currentGuide?.guide).toBe('issue');
expect(GuideStore.getState().currentGuide?.guide).toBe('trace_view');
// Should prune steps that don't have anchors.
expect(GuideStore.getState().currentGuide?.steps).toHaveLength(3);
expect(GuideStore.getState().currentGuide?.steps).toHaveLength(2);

GuideStore.nextStep();
expect(GuideStore.getState().currentStep).toBe(1);
GuideStore.nextStep();
expect(GuideStore.getState().currentStep).toBe(2);
GuideStore.closeGuide();
expect(GuideStore.getState().currentGuide).toBeNull();
});

it('should force show a guide with #assistant', () => {
data = [
{
guide: 'issue',
guide: 'issue_stream',
seen: true,
},
{guide: 'issue_stream', seen: false},
{guide: 'trace_view', seen: false},
];

GuideStore.fetchSucceeded(data);
window.location.hash = '#assistant';
window.dispatchEvent(new Event('load'));
expect(GuideStore.getState().currentGuide?.guide).toBe('issue');
GuideStore.closeGuide();
expect(GuideStore.getState().currentGuide?.guide).toBe('issue_stream');
GuideStore.closeGuide();
expect(GuideStore.getState().currentGuide?.guide).toBe('trace_view');
window.location.hash = '';
});

Expand All @@ -85,10 +82,10 @@ describe('GuideStore', () => {
it('should record analytics events when guide is cued', () => {
const spy = jest.spyOn(GuideStore, 'recordCue');
GuideStore.fetchSucceeded(data);
expect(spy).toHaveBeenCalledWith('issue');
expect(spy).toHaveBeenCalledWith('trace_view');

expect(trackAnalytics).toHaveBeenCalledWith('assistant.guide_cued', {
guide: 'issue',
guide: 'trace_view',
organization: null,
});

Expand All @@ -105,7 +102,7 @@ describe('GuideStore', () => {
it('only shows guides with server data and content', () => {
data = [
{
guide: 'issue',
guide: 'issue_stream',
seen: true,
},
{
Expand Down
4 changes: 2 additions & 2 deletions static/app/views/issueDetails/eventNavigation/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -246,9 +246,9 @@ export function IssueEventNavigation({event, group}: IssueEventNavigationProps)
<TourElement<IssueDetailsTour>
tourContext={IssueDetailsTourContext}
id={IssueDetailsTour.NAVIGATION}
title={t('Compare events')}
title={t('Compare and copy events')}
description={t(
'Review the events associated with an issue. Compare the first, latest, or recommended event to see what changed.'
'Review the events associated with an issue. Compare the first, latest, or recommended event to see what changed, or use Copy as to copy the issue details as Markdown.'
)}
>
{tourProps => (
Expand Down
2 changes: 2 additions & 0 deletions static/app/views/issueDetails/groupDetails.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ import {useMergedIssuesDrawer} from 'sentry/views/issueDetails/hooks/useMergedIs
import {useSimilarIssuesDrawer} from 'sentry/views/issueDetails/hooks/useSimilarIssuesDrawer';
import {
ISSUE_DETAILS_TOUR_GUIDE_KEY,
IssueDetailsTourModal,
IssueDetailsTourContext,
ORDERED_ISSUE_DETAILS_TOUR,
type IssueDetailsTour,
Expand Down Expand Up @@ -836,6 +837,7 @@ function GroupDetailsPageContent(props: GroupDetailsPageContentProps) {
orderedStepIds={ORDERED_ISSUE_DETAILS_TOUR}
TourContext={IssueDetailsTourContext}
>
<IssueDetailsTourModal />
<GroupDetailsContent
Comment on lines 837 to 841
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug: The issue details tour modal will not open for SOURCEMAP_CONFIGURATION and LOW_VALUE_SPAN_CONFIGURATION issues because some required tour steps are conditionally rendered and never register.
Severity: MEDIUM

Suggested Fix

Consider setting the requireAllStepsRegistered prop on TourContextProvider to false to allow the tour to run with a subset of steps. Alternatively, conditionally register tour steps based on the issue type's configuration, ensuring that only available steps are expected by the tour context for the tour to be considered 'registered'.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: static/app/views/issueDetails/groupDetails.tsx#L837-L841

Potential issue: The `TourContextProvider` is configured with
`requireAllStepsRegistered` set to `true`, meaning all tour steps must be registered
before the tour can start. However, for `SOURCEMAP_CONFIGURATION` and
`LOW_VALUE_SPAN_CONFIGURATION` issue types, the `filterBar` and `eventNavigation` are
disabled in their configuration. This prevents the `IssueDetailsTour.FILTERS` and
`IssueDetailsTour.NAVIGATION` steps from being rendered and registered. Consequently,
the `isRegistered` state in the tour context never becomes `true`. The
`useIssueDetailsTourModal` hook includes a check `if (!isRegistered || ...)` which then
prevents the tour modal from ever being displayed for these issue types, even when
forced via a URL hash.

Did we get this right? 👍 / 👎 to inform future reviews.

project={projectWithFallback}
group={props.group}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,7 @@ describe('groupEventDetails', () => {
initialRouterConfig,
});

expect(await screen.findByRole('region', {name: 'tags'})).toBeInTheDocument();
expect(await screen.findByRole('region', {name: 'Tags'})).toBeInTheDocument();
const highlights = screen.getByRole('region', {name: 'Highlights'});

expect(within(highlights).getByRole('button', {name: 'Edit'})).toBeInTheDocument();
Expand Down
Loading
Loading