-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
feat(replay): Web Vital Breadcrumb Design #76320
Changes from 16 commits
a4a432e
164696d
1518533
5276bbc
a631530
0705e5f
a04fed1
a81d020
5143a4c
ec938ae
65fe5b2
199c9eb
99eef0b
f0b23be
69c5ab9
e48cdb8
41e47cb
43aeaae
a347f86
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,10 @@ | ||
import type {CSSProperties, MouseEvent} from 'react'; | ||
import type {CSSProperties, ReactNode} from 'react'; | ||
import {isValidElement, memo, useCallback} from 'react'; | ||
import styled from '@emotion/styled'; | ||
import beautify from 'js-beautify'; | ||
|
||
import ProjectAvatar from 'sentry/components/avatar/projectAvatar'; | ||
import {Button} from 'sentry/components/button'; | ||
import {CodeSnippet} from 'sentry/components/codeSnippet'; | ||
import {Flex} from 'sentry/components/container/flex'; | ||
import ErrorBoundary from 'sentry/components/errorBoundary'; | ||
|
@@ -13,6 +14,7 @@ import PanelItem from 'sentry/components/panels/panelItem'; | |
import {OpenReplayComparisonButton} from 'sentry/components/replays/breadcrumbs/openReplayComparisonButton'; | ||
import {useReplayContext} from 'sentry/components/replays/replayContext'; | ||
import {useReplayGroupContext} from 'sentry/components/replays/replayGroupContext'; | ||
import StructuredEventData from 'sentry/components/structuredEventData'; | ||
import Timeline from 'sentry/components/timeline'; | ||
import {useHasNewTimelineUI} from 'sentry/components/timeline/utils'; | ||
import {Tooltip} from 'sentry/components/tooltip'; | ||
|
@@ -21,37 +23,37 @@ import {space} from 'sentry/styles/space'; | |
import type {Extraction} from 'sentry/utils/replays/extractHtml'; | ||
import {getReplayDiffOffsetsFromFrame} from 'sentry/utils/replays/getDiffTimestamps'; | ||
import getFrameDetails from 'sentry/utils/replays/getFrameDetails'; | ||
import useExtractDomNodes from 'sentry/utils/replays/hooks/useExtractDomNodes'; | ||
import type ReplayReader from 'sentry/utils/replays/replayReader'; | ||
import type { | ||
ErrorFrame, | ||
FeedbackFrame, | ||
HydrationErrorFrame, | ||
ReplayFrame, | ||
WebVitalFrame, | ||
} from 'sentry/utils/replays/types'; | ||
import { | ||
isBreadcrumbFrame, | ||
isErrorFrame, | ||
isFeedbackFrame, | ||
isHydrationErrorFrame, | ||
isSpanFrame, | ||
isWebVitalFrame, | ||
} from 'sentry/utils/replays/types'; | ||
import type {Color} from 'sentry/utils/theme'; | ||
import useOrganization from 'sentry/utils/useOrganization'; | ||
import useProjectFromSlug from 'sentry/utils/useProjectFromSlug'; | ||
import IconWrapper from 'sentry/views/replays/detail/iconWrapper'; | ||
import TimestampButton from 'sentry/views/replays/detail/timestampButton'; | ||
|
||
type MouseCallback = (frame: ReplayFrame, e: React.MouseEvent<HTMLElement>) => void; | ||
type MouseCallback = (frame: ReplayFrame, nodeId?: number) => void; | ||
|
||
const FRAMES_WITH_BUTTONS = ['replay.hydrate-error']; | ||
|
||
interface Props { | ||
frame: ReplayFrame; | ||
onClick: null | MouseCallback; | ||
onInspectorExpanded: ( | ||
path: string, | ||
expandedState: Record<string, boolean>, | ||
event: MouseEvent<HTMLDivElement> | ||
) => void; | ||
onInspectorExpanded: (path: string, expandedState: Record<string, boolean>) => void; | ||
onMouseEnter: MouseCallback; | ||
onMouseLeave: MouseCallback; | ||
startTimestampMs: number; | ||
|
@@ -105,15 +107,31 @@ function BreadcrumbItem({ | |
) : null; | ||
}, [frame, replay]); | ||
|
||
const renderCodeSnippet = useCallback(() => { | ||
return extraction?.html ? ( | ||
<CodeContainer> | ||
<CodeSnippet language="html" hideCopyButton> | ||
{beautify.html(extraction?.html, {indent_size: 2})} | ||
</CodeSnippet> | ||
</CodeContainer> | ||
const renderWebVital = useCallback(() => { | ||
return isSpanFrame(frame) && isWebVitalFrame(frame) ? ( | ||
<WebVitalData | ||
replay={replay} | ||
frame={frame} | ||
expandPaths={expandPaths} | ||
onInspectorExpanded={onInspectorExpanded} | ||
onMouseEnter={onMouseEnter} | ||
onMouseLeave={onMouseLeave} | ||
/> | ||
) : null; | ||
}, [extraction?.html]); | ||
}, [expandPaths, frame, onInspectorExpanded, onMouseEnter, onMouseLeave, replay]); | ||
|
||
const renderCodeSnippet = useCallback(() => { | ||
return ( | ||
(!isSpanFrame(frame) || (isSpanFrame(frame) && !isWebVitalFrame(frame))) && | ||
extraction?.html?.map(html => ( | ||
<CodeContainer key={html}> | ||
<CodeSnippet language="html" hideCopyButton> | ||
{beautify.html(html, {indent_size: 2})} | ||
</CodeSnippet> | ||
</CodeContainer> | ||
)) | ||
); | ||
}, [extraction?.html, frame]); | ||
|
||
const renderIssueLink = useCallback(() => { | ||
return isErrorFrame(frame) || isFeedbackFrame(frame) ? ( | ||
|
@@ -143,13 +161,17 @@ function BreadcrumbItem({ | |
data-is-error-frame={isErrorFrame(frame)} | ||
style={style} | ||
className={className} | ||
onClick={e => onClick?.(frame, e)} | ||
onMouseEnter={e => onMouseEnter(frame, e)} | ||
onMouseLeave={e => onMouseLeave(frame, e)} | ||
onClick={event => { | ||
event.stopPropagation(); | ||
onClick?.(frame); | ||
}} | ||
onMouseEnter={() => onMouseEnter(frame)} | ||
onMouseLeave={() => onMouseLeave(frame)} | ||
> | ||
<ErrorBoundary mini> | ||
{renderDescription()} | ||
{renderComparisonButton()} | ||
{renderWebVital()} | ||
{renderCodeSnippet()} | ||
{renderIssueLink()} | ||
</ErrorBoundary> | ||
|
@@ -160,9 +182,12 @@ function BreadcrumbItem({ | |
<CrumbItem | ||
data-is-error-frame={isErrorFrame(frame)} | ||
as={onClick && !forceSpan ? 'button' : 'span'} | ||
onClick={e => onClick?.(frame, e)} | ||
onMouseEnter={e => onMouseEnter(frame, e)} | ||
onMouseLeave={e => onMouseLeave(frame, e)} | ||
onClick={event => { | ||
event.stopPropagation(); | ||
onClick?.(frame); | ||
}} | ||
onMouseEnter={() => onMouseEnter(frame)} | ||
onMouseLeave={() => onMouseLeave(frame)} | ||
style={style} | ||
className={className} | ||
> | ||
|
@@ -184,6 +209,7 @@ function BreadcrumbItem({ | |
{renderDescription()} | ||
</Flex> | ||
{renderComparisonButton()} | ||
{renderWebVital()} | ||
{renderCodeSnippet()} | ||
{renderIssueLink()} | ||
</CrumbDetails> | ||
|
@@ -192,6 +218,113 @@ function BreadcrumbItem({ | |
); | ||
} | ||
|
||
function WebVitalData({ | ||
replay, | ||
frame, | ||
expandPaths, | ||
onInspectorExpanded, | ||
onMouseEnter, | ||
onMouseLeave, | ||
}: { | ||
expandPaths: string[] | undefined; | ||
frame: WebVitalFrame; | ||
onInspectorExpanded: (path: string, expandedState: Record<string, boolean>) => void; | ||
onMouseEnter: MouseCallback; | ||
onMouseLeave: MouseCallback; | ||
replay: ReplayReader | null; | ||
}) { | ||
// TODO: remove test CLS data once SDK is merged and updated | ||
const clsFrame = { | ||
...frame, | ||
data: { | ||
value: frame.data.value, | ||
size: frame.data.size, | ||
rating: frame.data.rating, | ||
nodeIds: [frame.data.nodeIds, 333, 870], | ||
attributes: [ | ||
{value: 0.0123, nodeIds: frame.data.nodeIds ?? [93]}, | ||
{value: 0.0345, nodeIds: [333, 870]}, | ||
], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. some magic numbers will need to be removed tho |
||
}, | ||
}; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. to be removed once the sdk is updated There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are older sdk versions sending data in this format? if so we'd need to continue to support it as there are customers out there on that older version There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, the older format is supported too! This is just to demo what it would look like with the newer format since we don't have that yet, with the older format, it'll look more similar to the other web vitals (eg. no layout shifts, only element and value). |
||
const {data: frameToExtraction} = useExtractDomNodes({replay}); | ||
const selectors = frameToExtraction?.get(frame)?.selectors; | ||
|
||
const webVitalData = {value: frame.data.value}; | ||
if ( | ||
frame.description === 'cumulative-layout-shift' && | ||
'attributes' in frame.data && | ||
selectors | ||
) { | ||
const layoutShifts: {[x: string]: ReactNode[]}[] = []; | ||
for (const attr of clsFrame.data.attributes) { | ||
const elements: ReactNode[] = []; | ||
attr.nodeIds?.forEach(nodeId => { | ||
selectors.get(nodeId) | ||
? elements.push( | ||
<span | ||
key={nodeId} | ||
onMouseEnter={() => onMouseEnter(clsFrame, nodeId)} | ||
onMouseLeave={() => onMouseLeave(clsFrame, nodeId)} | ||
> | ||
<ValueObjectKey>{t('element')}</ValueObjectKey> | ||
<span>{': '}</span> | ||
<span> | ||
<SelectorButton>{selectors.get(nodeId)}</SelectorButton> | ||
</span> | ||
</span> | ||
) | ||
: null; | ||
}); | ||
// if we can't find the elements associated with the layout shift, we still show the score with element: unknown | ||
if (!elements.length) { | ||
elements.push( | ||
<span> | ||
<ValueObjectKey>{t('element')}</ValueObjectKey> | ||
<span>{': '}</span> | ||
<ValueNull>{t('unknown')}</ValueNull> | ||
</span> | ||
); | ||
} | ||
layoutShifts.push({[`score ${attr.value}`]: elements}); | ||
} | ||
if (layoutShifts.length) { | ||
webVitalData['Layout shifts'] = layoutShifts; | ||
} | ||
} else if (selectors?.size) { | ||
selectors.forEach((key, value) => { | ||
webVitalData[key] = ( | ||
<span | ||
key={key} | ||
onMouseEnter={() => onMouseEnter(frame, value)} | ||
onMouseLeave={() => onMouseLeave(frame, value)} | ||
> | ||
<ValueObjectKey>{t('element')}</ValueObjectKey> | ||
<span>{': '}</span> | ||
<SelectorButton size="zero" borderless> | ||
{key} | ||
</SelectorButton> | ||
</span> | ||
); | ||
}); | ||
} | ||
|
||
return ( | ||
<StructuredEventData | ||
initialExpandedPaths={expandPaths ?? []} | ||
onToggleExpand={(expandedPaths, path) => { | ||
onInspectorExpanded( | ||
path, | ||
Object.fromEntries(expandedPaths.map(item => [item, true])) | ||
); | ||
}} | ||
data={webVitalData} | ||
withAnnotatedText | ||
/> | ||
); | ||
} | ||
|
||
function CrumbHydrationButton({ | ||
replay, | ||
frame, | ||
|
@@ -381,4 +514,27 @@ const CodeContainer = styled('div')` | |
overflow: auto; | ||
`; | ||
|
||
const ValueObjectKey = styled('span')` | ||
color: var(--prism-keyword); | ||
`; | ||
|
||
const ValueNull = styled('span')` | ||
font-weight: ${p => p.theme.fontWeightBold}; | ||
color: var(--prism-property); | ||
`; | ||
|
||
const SelectorButton = styled(Button)` | ||
background: none; | ||
border: none; | ||
padding: 0 2px; | ||
border-radius: 2px; | ||
font-weight: ${p => p.theme.fontWeightNormal}; | ||
box-shadow: none; | ||
font-size: ${p => p.theme.fontSizeSmall}; | ||
color: ${p => p.theme.subText}; | ||
margin: 0 ${space(0.5)}; | ||
height: auto; | ||
min-height: auto; | ||
`; | ||
|
||
Comment on lines
+513
to
+525
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. might be able to get away with something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. either way i don't mind! |
||
export default memo(BreadcrumbItem); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Isn't the second
isSpanFrame()
check redundant? e.g.not a OR (a AND b)
==not A OR b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great catch! boolean logic hurts my brain :(