feat(issue-details): Revise Context UI behind feature flag #68081

leeandher · 2024-04-02T17:04:26Z

Requires #68530

This PR changes the interface for displaying context items on the issue details page behind the event-tags-tree-ui flag.

Requires https://github.com/getsentry/sentry/pull/68530 to create an easy interface to access the formatted context data outside of the ContextBlock components.

The masonry layout will be deferred for now to focus on highlights a bit more.

Before

After

todo

Add tests
Add screenshots
Double check + screenshots for partial redaction
Double check + screenshots for data scrubbing rules as 'error state'

update

Okay, was able to double check masking and extremely large values and made some small changes for formatting. Advanced data scrubbing still still shows up with UX friendly tooltips, and I think it's okay they don't show up as errors since they're expected by the organization.

codecov · 2024-04-03T17:26:04Z

Bundle Report

Changes will increase total bundle size by 2.73kB ⬆️

Bundle name	Size	Change
sentry-webpack-bundle-array-push	26.29MB	2.73kB ⬆️

Separating this out from #68081 since it is a wider change now. Doing this will help simplify the above PR greatly, since it surfaces easy access to context titles and meta values (which store any error states from processing) - `getTitle` function was just moved to utils as is, and renamed to `getContextTitle` - `getContextMeta` was introduced and implemented for all known context sections Here is the meta still being rendered as expected: ![image](https://github.com/getsentry/sentry/assets/35509934/b93d849a-4adf-4fcf-9077-04021c40b6da)

…68530) Another PR similar to #68349 to help with simplifying #68081 The goal of this PR is to extract the logic for how we display the known/unknown key value pairs for context items into their own functions and create a util function to easily receive a list of `KeyValueListItem[]` objects which we'll be able to render in our new component. This will prevent them from diverging while we're iterating on the new UI. I had to do this in the verbose helper function way since the logic for converting context keys (e.g. `transaction.op` to `t('Operation Name')`) is unique for EACH context, and lived within the component which rendered the data. Now the data can be accessed outside that specific component, which will make swapping it much easier. It also now defaults to the `raw` data, rather than returning `<StructuredEventData />` components which make the value inaccessible. That formatting can be accessed by calling `getKnownStructuredData` on the result of `getKnownData`. todo - [x] Add test for `getKnownStructuredData` - [x] Ensure CI passes

scttcper · 2024-04-15T16:40:06Z

static/app/components/events/contexts/contextCard.spec.tsx

+      book: {
+        title: 'This Is How You Lose the Time War',
+        pages: 208,
+        published: new Date('2018-07-21'),


would this be a date or an iso string from the api

Good catch, yeah the API returns ISO strings -- I'll update this.

scttcper

looks good. css masonry grid layout plz browsers

malwilley · 2024-04-15T16:33:21Z

static/app/components/events/contexts/contextCard.tsx

+          depth={0}
+          maxDefaultDepth={0}
+          meta={contextMeta}
+          config={{}}


nit: no need to pass config, it's not required

It's typed as

config: StructedEventDataConfig | undefined

so its required to declare it. I'll update the type to:

config?: StructuredEventDataConfig

so that I don't have to set it here, and nothing should change.

Sorry, I didn't realize that inner component was being exported. I added the | undefined because it's easy to forget one of the props when you are doing a recursive render. But for top-level components it's nice to keep the API minimal

malwilley · 2024-04-15T16:43:45Z

static/app/components/events/meta/annotatedText/index.tsx

+  value,
+  meta,
+  className,
+  withOnlyFormattedText = false,


Are you adding this prop to remove the formatting so that you can apply error states to the whole key/value row? I'm not sure if that's the best idea since the annotated text is sometimes only applied to sections of text and not the entire value. It also seems weird to keep a tooltip but not have the highlighted text so you know where to hover.

Such as for size limits, how would this be handled?

I didn't realize this was something we did. At least as per designs, the background was removed for the <invalid>/<redacted> values which was my intention, but you're right that if partially redacted text has a tooltip, we would want to retain it. I'll test this locally and update.

IMO you might want to sync with Vu and see if this requirement can be modified. It would be a lot simpler and more consistent to just keep the annotations the same as they are in the rest of the page, so if that's an option that would be my preference.

malwilley · 2024-04-15T16:45:18Z

static/app/components/events/contexts/contextCard.tsx

+          meta={contextMeta}
+          config={{}}
+          withAnnotatedText
+          withOnlyFormattedText


Instead of passing withOnlyFormattedText is it possible to just pass withAnnotatedText={false}?

We still want to have annotated text, we just don't want some of the styling that comes packaged with them. This is for transforming null into italicized <invalid> or <redacted> text. The intention is that this new prop spits out primitive text that we can style now, rather than the shaded text with icon tooltips by default (since the design required us moving that to the end of the row)

malwilley · 2024-04-15T16:52:35Z

static/app/components/events/contexts/contextCard.tsx

+            ) : (
+              dataComponent
+            )}
+            <AnnotatedTextErrors errors={contextErrors} />


I believe that this will only show errors, but not things like data scrubbing? I'd test with an event that has scrubbed values or values that are too long to see what that looks like. We still want those scrubbed values to be highlighted in some way.

That's a good point, I will look into this before merging and add a screenshot/update code if needed. Thanks!

leeandher

Will update as per review comments before merging

leeandher · 2024-04-15T17:32:51Z

static/app/components/events/contexts/contextCard.spec.tsx

+      book: {
+        title: 'This Is How You Lose the Time War',
+        pages: 208,
+        published: new Date('2018-07-21'),


Good catch, yeah the API returns ISO strings -- I'll update this.

leeandher · 2024-04-15T17:35:28Z

static/app/components/events/contexts/contextCard.tsx

+          depth={0}
+          maxDefaultDepth={0}
+          meta={contextMeta}
+          config={{}}


It's typed as

config: StructedEventDataConfig | undefined

so its required to declare it. I'll update the type to:

config?: StructuredEventDataConfig

so that I don't have to set it here, and nothing should change.

leeandher · 2024-04-15T17:38:48Z

static/app/components/events/contexts/contextCard.tsx

+          meta={contextMeta}
+          config={{}}
+          withAnnotatedText
+          withOnlyFormattedText


We still want to have annotated text, we just don't want some of the styling that comes packaged with them. This is for transforming null into italicized <invalid> or <redacted> text. The intention is that this new prop spits out primitive text that we can style now, rather than the shaded text with icon tooltips by default (since the design required us moving that to the end of the row)

leeandher · 2024-04-15T17:39:36Z

static/app/components/events/contexts/contextCard.tsx

+            ) : (
+              dataComponent
+            )}
+            <AnnotatedTextErrors errors={contextErrors} />


That's a good point, I will look into this before merging and add a screenshot/update code if needed. Thanks!

leeandher · 2024-04-15T17:42:07Z

static/app/components/events/meta/annotatedText/index.tsx

+  value,
+  meta,
+  className,
+  withOnlyFormattedText = false,


I didn't realize this was something we did. At least as per designs, the background was removed for the <invalid>/<redacted> values which was my intention, but you're right that if partially redacted text has a tooltip, we would want to retain it. I'll test this locally and update.

malwilley

Looks good!

leeandher added the WIP label Apr 2, 2024

github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Apr 2, 2024

vercel bot deployed to Preview April 2, 2024 17:06 View deployment

vercel bot deployed to Preview April 3, 2024 17:23 View deployment

leeandher mentioned this pull request Apr 5, 2024

chore(issue-details): Move logic into utils #68349

Merged

vercel bot deployed to Preview April 5, 2024 17:14 View deployment

vercel bot deployed to Preview April 5, 2024 19:45 View deployment

vercel bot deployed to Preview April 8, 2024 16:20 View deployment

vercel bot deployed to Preview April 9, 2024 13:46 View deployment

leeandher mentioned this pull request Apr 9, 2024

ref(issue-details): Refactor context items into separate functions #68530

Merged

2 tasks

leeandher changed the base branch from master to leander/ref-context-items April 9, 2024 17:39

leeandher force-pushed the leander/context-changes branch from 63a1b9d to 0164c3d Compare April 9, 2024 17:43

vercel bot deployed to Preview April 9, 2024 17:45 View deployment

leeandher force-pushed the leander/context-changes branch from 0164c3d to 33f222a Compare April 9, 2024 17:46

vercel bot deployed to Preview April 9, 2024 17:48 View deployment

vercel bot deployed to Preview April 9, 2024 19:19 View deployment

vercel bot deployed to Preview April 10, 2024 14:01 View deployment

leeandher removed the WIP label Apr 10, 2024

vercel bot deployed to Preview April 10, 2024 14:15 View deployment

leeandher force-pushed the leander/context-changes branch from 4718087 to 15f169d Compare April 10, 2024 17:32

vercel bot deployed to Preview April 10, 2024 17:35 View deployment

Base automatically changed from leander/ref-context-items to master April 10, 2024 19:22

leeandher added 4 commits April 10, 2024 15:43

✨ Squash changes for simplicity

4eb5b2e

💄 Ensure column count matches the tag tree above

742fa6d

✨ Allow annotated text for tags

3309e1b

✨ Allow annotated text for context

d21e219

leeandher added 2 commits April 10, 2024 15:43

💄 hide contexts which only have 'type' key

ebb552c

✨ Ensure trace/profile have links

f1c4e69

leeandher force-pushed the leander/context-changes branch from 15f169d to f1c4e69 Compare April 11, 2024 14:45

vercel bot deployed to Preview April 11, 2024 14:49 View deployment

Merge branch 'master' into leander/context-changes

86d9237

vercel bot deployed to Preview April 11, 2024 15:23 View deployment

✅ Don't change casing on unknown tags

1c2cdc9

vercel bot deployed to Preview April 11, 2024 15:29 View deployment

✅ Add test

69a1255

leeandher requested a review from a team April 11, 2024 17:07

leeandher marked this pull request as ready for review April 11, 2024 17:07

vercel bot deployed to Preview April 11, 2024 17:09 View deployment

✨ Ensure unique key to prevent log spew

e94bc2b

vercel bot deployed to Preview April 15, 2024 15:05 View deployment

scttcper reviewed Apr 15, 2024

View reviewed changes

scttcper approved these changes Apr 15, 2024

View reviewed changes

malwilley reviewed Apr 15, 2024

View reviewed changes

leeandher commented Apr 15, 2024

View reviewed changes

♻️ Account for extremely long values

7fbf636

vercel bot deployed to Preview April 15, 2024 19:56 View deployment

🔥 rm background changes

9002e72

vercel bot deployed to Preview April 15, 2024 20:24 View deployment

malwilley approved these changes Apr 15, 2024

View reviewed changes

leeandher merged commit 530e806 into master Apr 15, 2024
42 checks passed

leeandher deleted the leander/context-changes branch April 15, 2024 20:36

github-actions bot locked and limited conversation to collaborators May 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(issue-details): Revise Context UI behind feature flag #68081

feat(issue-details): Revise Context UI behind feature flag #68081

leeandher commented Apr 2, 2024 •

edited

codecov bot commented Apr 3, 2024 •

edited

scttcper Apr 15, 2024

leeandher Apr 15, 2024

scttcper left a comment

malwilley Apr 15, 2024

leeandher Apr 15, 2024

malwilley Apr 15, 2024

malwilley Apr 15, 2024

leeandher Apr 15, 2024

malwilley Apr 15, 2024

malwilley Apr 15, 2024

leeandher Apr 15, 2024

malwilley Apr 15, 2024

leeandher Apr 15, 2024

leeandher left a comment

leeandher Apr 15, 2024

leeandher Apr 15, 2024

leeandher Apr 15, 2024

leeandher Apr 15, 2024

leeandher Apr 15, 2024

malwilley left a comment

feat(issue-details): Revise Context UI behind feature flag #68081

feat(issue-details): Revise Context UI behind feature flag #68081

Conversation

leeandher commented Apr 2, 2024 • edited

codecov bot commented Apr 3, 2024 • edited

Bundle Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scttcper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leeandher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malwilley left a comment

Choose a reason for hiding this comment

leeandher commented Apr 2, 2024 •

edited

codecov bot commented Apr 3, 2024 •

edited