Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ref(issues): rm replay id from event highlights #76316

Closed
wants to merge 5 commits into from
Closed

ref(issues): rm replay id from event highlights #76316

wants to merge 5 commits into from

Conversation

michellewzhang
Copy link
Member

@michellewzhang michellewzhang commented Aug 16, 2024 •
edited
Loading

remove replay id from the issue details event highlights edit modal. it also removes replay id (both tag and context options) from the view if it was previously pinned by any user.

see also

reasoning for removing

  • we have the replay preview in the issue details if it exists, and replay ID doesn't add much useful context
  • sometimes the replay ID link leads to an invalid replay ("replay not found" error) because it was dropped, deleted, etc. and this component doesn't take that into account before rendering the link

before

SCR-20240815-ozuw

after

SCR-20240815-ozsh

@michellewzhang michellewzhang requested a review from a team as a code owner August 16, 2024 00:07
@michellewzhang michellewzhang requested review from leeandher, jas-kas and a team August 16, 2024 00:07
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Aug 16, 2024
@getsentry getsentry deleted a comment from codecov bot Aug 16, 2024
michellewzhang
michellewzhang commented Aug 16, 2024
View reviewed changes
static/app/components/events/highlights/highlightsDataSection.tsx Outdated
@@ -165,7 +170,11 @@ function HighlightsData({
[]
);

const highlightTagItems = getHighlightTagData({event, highlightTags});
const highlightTagItems = getHighlightTagData({
Copy link
Member Author

Choose a reason for hiding this comment

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

this removes it from the UI if the user has previously pinned replayId

Copy link
Member

Choose a reason for hiding this comment

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

I this approach works to avoid people clicking dead links to replays but these highlights are set by users which means we'll be going against their wishes for displaying the link at the top of the page. Looking at the DB, there's about 26 projects in SaaS which have set replayId or replay_id to appear in their highlights.

Is there any harm in retaining these links for highlights? I think it might make more sense to figure out a way to prevent linking to missing replays and instead display some mention that the replay wasn't found rather than omitting it entirely.

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @jas-kas

Copy link
Member

Choose a reason for hiding this comment

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

Is there any harm in retaining these links for highlights? I think it might make more sense to figure out a way to prevent linking to missing replays and instead display some mention that the replay wasn't found rather than omitting it entirely.

I'm okay with retaining these links in Issue Details highlights. Are you suggesting to keep them there if the user has pinned replay_id or in general for Issue Details highlights (for all users)? I'm good with either case but want to confirm the behaviour.

I think it might make more sense to figure out a way to prevent linking to missing replays and instead display some mention that the replay wasn't found rather than omitting it entirely.

This would be ideal (in the world where a user has explicitly pinned replay_id) -- that's what we do in the Replay section inside Issue Details as you know. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed in #76619

michellewzhang
michellewzhang commented Aug 16, 2024
View reviewed changes
static/app/components/events/highlights/highlightsDataSection.tsx Outdated
const highlightContextDataItems = getHighlightContextData({
event,
project,
organization,
highlightContext,
highlightContext: {
Copy link
Member Author

Choose a reason for hiding this comment

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

this removes it from the UI if the user has previously pinned replay_id

michellewzhang
michellewzhang commented Aug 16, 2024
View reviewed changes
static/app/components/events/highlights/editHighlightsModal.tsx Outdated
useState<HighlightContext>(prevHighlightContext);
const [highlightTags, setHighlightTags] = useState<HighlightTags>(prevHighlightTags);
const {replay, ...rest} = prevHighlightContext;
const [highlightContext, setHighlightContext] = useState<HighlightContext>({
Copy link
Member Author

@michellewzhang michellewzhang Aug 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this removes replay_id from the preview UI up top when in the highlight modal, if the user has previously pinned it

michellewzhang
michellewzhang commented Aug 16, 2024
View reviewed changes
static/app/components/events/highlights/editHighlightsModal.tsx Outdated
...(replay && {replay: replay?.filter(k => k !== 'replay_id')}),
...rest,
});
const [highlightTags, setHighlightTags] = useState<HighlightTags>(
Copy link
Member Author

Choose a reason for hiding this comment

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

this removes replayId from the preview UI up top when in the highlight modal, if the user has previously pinned it

michellewzhang
michellewzhang commented Aug 16, 2024
View reviewed changes
static/app/components/events/highlights/editHighlightsModal.tsx Outdated
const filteredContextKeys = contextKeys.filter(
contextKey => contextKey.includes(ctxFilter) || contextType.includes(ctxFilter)
contextKey =>
Copy link
Member Author

Choose a reason for hiding this comment

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

this removes replay_id as an addable option

michellewzhang
michellewzhang commented Aug 16, 2024
View reviewed changes
static/app/components/events/highlights/editHighlightsModal.tsx Outdated
const tagData = event.tags
.filter(tag => tag.key?.includes(tagFilter))
.filter(tag => tag.key?.includes(tagFilter) && tag.key !== 'replayId')
Copy link
Member Author

Choose a reason for hiding this comment

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

this removes replayId as an addable option

@michellewzhang michellewzhang mentioned this pull request Aug 27, 2024
Merged
michellewzhang added a commit that referenced this pull request Aug 27, 2024
@michellewzhang
fix(issues/replay): if fetchError, don't render replayId in highlights (
2304d9b 
#76619)

redo of #76316: 

instead of completely removing the replay ID from the event highlights,
we now do an extra check to see if there was a `fetchError` with this
replay ID (e.g. whether this ID is actually valid or not). if it's not
valid, then don't render the replay ID to avoid linking to an invalid
replay. the link will take you to a page like this:

<img width="1126" alt="SCR-20240827-kqxv"
src="https://github.com/user-attachments/assets/d5dc991f-e649-4ada-8a64-64db8fc94a50">

before: issue details replay processing error (blue banner shows up) but
the event highlights still contains the replay ID
<img width="880" alt="SCR-20240827-krda"
src="https://github.com/user-attachments/assets/c4a0cbd6-9143-4043-b3f2-0516704999dd">
<img width="897" alt="SCR-20240827-kqac"
src="https://github.com/user-attachments/assets/cb802d18-9dbf-47ee-99ab-dcd4baf5e2e0">


after: replayId is shown as `--` and no link
<img width="795" alt="SCR-20240827-kpwz"
src="https://github.com/user-attachments/assets/709d0c64-9779-4e4b-8641-49d1cc3359cb">
@michellewzhang michellewzhang deleted the mz/highlights-rm branch August 29, 2024 20:43
@github-actions github-actions bot locked and limited conversation to collaborators Sep 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jas-kas jas-kas jas-kas approved these changes

@JoshFerge JoshFerge JoshFerge approved these changes

@leeandher leeandher Awaiting requested review from leeandher

Assignees
No one assigned
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@michellewzhang @JoshFerge @leeandher @jas-kas