-
-
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): Use SDK value for LCP #44868
Changes from all commits
229345e
fbf95f8
4c833df
c675dc5
6f71a8b
8a06d23
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,14 +1,30 @@ | ||
import {Tooltip} from 'sentry/components/tooltip'; | ||
import {IconWarning} from 'sentry/icons'; | ||
import {t} from 'sentry/locale'; | ||
import {BreadcrumbType, Crumb} from 'sentry/types/breadcrumbs'; | ||
|
||
/** | ||
* Generate breadcrumb descriptions based on type | ||
*/ | ||
export function getDescription(crumb: Crumb, startTimestampMs?: number) { | ||
export function getDescription(crumb: Crumb) { | ||
if (typeof crumb.data === 'object' && crumb.data !== null && 'action' in crumb.data) { | ||
switch (crumb.data.action) { | ||
case 'largest-contentful-paint': | ||
if (crumb.timestamp && startTimestampMs) { | ||
return `${new Date(crumb.timestamp).getTime() - startTimestampMs}ms`; | ||
if (crumb.data?.value !== undefined) { | ||
return `${Math.round(crumb.data.value)}ms`; | ||
} | ||
if (crumb.data?.duration !== undefined) { | ||
// this means user is using an old SDK where LCP values are not | ||
// always correct. Prompt them to upgrade | ||
return ( | ||
<Tooltip | ||
title={t( | ||
'This replay uses a SDK version that is subject to inaccurate LCP values. Please upgrade to the latest version for best results if you have not already done so.' | ||
)} | ||
> | ||
<IconWarning /> | ||
</Tooltip> | ||
); | ||
Comment on lines
+16
to
+27
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. This is new since last PR -- ss in PR desc. |
||
} | ||
break; | ||
default: | ||
|
@@ -50,6 +66,6 @@ export function getTitle(crumb: Crumb) { | |
/** | ||
* Generate breadcrumb title + descriptions | ||
*/ | ||
export function getDetails(crumb: Crumb, startTimestampMs?: number) { | ||
return {title: getTitle(crumb), description: getDescription(crumb, startTimestampMs)}; | ||
export function getDetails(crumb: Crumb) { | ||
return {title: getTitle(crumb), description: getDescription(crumb)}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ import styled from '@emotion/styled'; | |
import BreadcrumbIcon from 'sentry/components/events/interfaces/breadcrumbs/breadcrumb/type/icon'; | ||
import {PanelItem} from 'sentry/components/panels'; | ||
import {getDetails} from 'sentry/components/replays/breadcrumbs/utils'; | ||
import {Tooltip} from 'sentry/components/tooltip'; | ||
import {SVGIconProps} from 'sentry/icons/svgIcon'; | ||
import {space} from 'sentry/styles/space'; | ||
import type {Crumb} from 'sentry/types/breadcrumbs'; | ||
|
@@ -32,7 +33,7 @@ function BreadcrumbItem({ | |
onMouseLeave, | ||
onClick, | ||
}: Props) { | ||
const {title, description} = getDetails(crumb, startTimestampMs); | ||
const {title, description} = getDetails(crumb); | ||
|
||
const handleMouseEnter = useCallback( | ||
(e: React.MouseEvent<HTMLElement>) => onMouseEnter && onMouseEnter(crumb, e), | ||
|
@@ -74,7 +75,9 @@ function BreadcrumbItem({ | |
) : null} | ||
</TitleContainer> | ||
|
||
<Description title={description}>{description}</Description> | ||
<Tooltip title={description} showOnlyOnOverflow> | ||
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. Previously it was expecting a string, now it uses |
||
<Description>{description}</Description> | ||
</Tooltip> | ||
</CrumbDetails> | ||
</CrumbItem> | ||
); | ||
|
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.
just float things