Skip to content

ref(related_issues): Improve code for title, subtitle and message#73053

Closed
armenzg wants to merge 10 commits into
masterfrom
fix/title/related_issue/armenzg
Closed

ref(related_issues): Improve code for title, subtitle and message#73053
armenzg wants to merge 10 commits into
masterfrom
fix/title/related_issue/armenzg

Conversation

@armenzg

@armenzg armenzg commented Jun 20, 2024

Copy link
Copy Markdown
Member

This fixes the rendering of some issues to match what we do on the Issue Details page.

Changes:

  • Fix default events
  • Fix some error events edge cases using culprit and error.value information

image

This shows a working default event:
image
instead of this:
image

NOTE: We can't use the same function we use for the Issue Details page since the data come from different endpoints and have different types.

@armenzg armenzg self-assigned this Jun 20, 2024
@github-actions github-actions Bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jun 20, 2024
@codecov

codecov Bot commented Jun 20, 2024

Copy link
Copy Markdown

Bundle Report

Changes will decrease total bundle size by 16.5kB ⬇️

Bundle name Size Change
app-webpack-bundle-array-push 27.3MB 16.5kB ⬇️

@armenzg armenzg force-pushed the fix/title/related_issue/armenzg branch from 1d3f5a1 to bee438f Compare June 21, 2024 18:42
Comment thread static/app/views/issueDetails/traceTimeline/traceIssue.tsx Outdated
Comment thread static/app/views/issueDetails/traceTimeline/traceIssue.tsx
Comment thread static/app/views/issueDetails/traceTimeline/traceIssue.tsx Outdated
@armenzg armenzg force-pushed the fix/title/related_issue/armenzg branch from bee438f to cc3e1e3 Compare July 2, 2024 19:02
@armenzg armenzg changed the base branch from master to feat/post_ga_work/related_issues/armenzg July 2, 2024 19:02
Comment thread static/app/views/issueDetails/traceTimeline/traceIssue.tsx
Base automatically changed from feat/post_ga_work/related_issues/armenzg to master July 3, 2024 11:46
@armenzg armenzg force-pushed the fix/title/related_issue/armenzg branch from cc3e1e3 to afd9492 Compare July 3, 2024 17:56
@codecov

codecov Bot commented Jul 3, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 78.13%. Comparing base (42068cb) to head (afd9492).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #73053      +/-   ##
==========================================
- Coverage   78.13%   78.13%   -0.01%     
==========================================
  Files        6652     6652              
  Lines      297597   297598       +1     
  Branches    51204    51205       +1     
==========================================
- Hits       232527   232526       -1     
- Misses      58766    58768       +2     
  Partials     6304     6304              
Files Coverage Δ
...ueDetails/traceTimeline/useTraceTimelineEvents.tsx 93.54% <ø> (ø)
...pp/views/issueDetails/traceTimeline/traceIssue.tsx 85.29% <87.50%> (-5.62%) ⬇️

armenzg added 5 commits July 4, 2024 09:08
This change tries to bring the code closer to what the Issues Details UI does for getting the title, subtitle and message displayed.
Comment thread static/app/views/issueDetails/traceTimeline/traceIssue.tsx Outdated
try {
title = event.title.trimEnd();
if (event['event.type'] === 'error') {
if (title[title.length - 1] !== ':') {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This handles the case when the colon is still part of the title:
image

// This is a case where the culprit is available while the transaction is not
transaction: '',
'event.type': event.type,
'stack.function': [],

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Typing is now actually working. I was not warned about this until now.

);
});

it('trace-related: check title, subtitle for error event', async () => {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Since the code path to generating is different than an issue platform event we need this test.

<NoOverflowDiv>
<TraceIssueEventTitle>{title}</TraceIssueEventTitle>
<TraceIssueEventSubtitle>{subtitle}</TraceIssueEventSubtitle>
<TraceIssueEventSubtitle data-test-id="subtitle-span">

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This will enable a test for the default event type (which is the empty string).

}
// https://github.com/getsentry/sentry/blob/f08644a004f9980d48f93dec2d7cfc9eeecd9a9e/static/app/utils/events.tsx#L48-L50
// It uses metadata.value which could differ depending on what error.value is used in the event manager
message = event['error.value'][event['error.value'].length - 1];

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The case I'm thinking of is ANR mobile issues where the stack trace is reversed, thus, the title could come from the other end.

I'm still comfortable going this way as this would fix the majority of all other platforms.

// https://github.com/getsentry/sentry/blob/f08644a004f9980d48f93dec2d7cfc9eeecd9a9e/static/app/utils/events.tsx#L48-L50
// It uses metadata.value which could differ depending on what error.value is used in the event manager
message = event['error.value'][event['error.value'].length - 1];
} else if (event['event.type'] === 'default') {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is new support.

const issuePlatformEvent = event as TimelineIssuePlatformEvent;
// It is suspected that this value is calculated somewhere in Relay
// and we deconstruct it here to match what the Issue details page shows
message = issuePlatformEvent.message

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is as it was:
image

transaction: string;
}

interface TimelineDiscoverEvent extends BaseEvent {}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was incorrect as it should have contained event.type and stack.function.

expect(await screen.findByText('The last error value')).toBeInTheDocument();
});

it('trace-related: check title, subtitle for default event', async () => {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The test for the default event.

// It is suspected that this value is calculated somewhere in Relay
// and we deconstruct it here to match what the Issue details page shows
message = event.message.replace(event.transaction, '').replace(title, '');
let subtitle = event.culprit || '';

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This moves us away from event.transaction:
image

getTitle in events.tsx sets the value to these:

subtitle: culprit,

subtitle: isIssue ? culprit : '',

@armenzg

armenzg commented Jul 5, 2024

Copy link
Copy Markdown
Member Author

@armenzg armenzg closed this Jul 5, 2024
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 21, 2024
@armenzg armenzg deleted the fix/title/related_issue/armenzg branch June 5, 2025 14:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant