-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(preprod): add cards to build details #103470
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
Conversation
| } | ||
|
|
||
| function getWatchBreakdown( | ||
| primaryMetric: ReturnType<typeof getMainArtifactSizeMetric>, |
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.
What is this? Just type this as BuildDetailsSizeInfoSizeMetric | undefined
| export function BuildDetailsMetricCards(props: BuildDetailsMetricCardsProps) { | ||
| const {sizeInfo, processedInsights, totalSize, platform, onOpenInsightsSidebar} = props; | ||
|
|
||
| const labels = getLabels(platform ?? undefined); |
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.
Alot of this safety here (i.e. ?? undefined) and the checks below is quite tough to read. I'd recommend destructuring a bit, or early returns/error states if things are missing.
For example, I see sizeInfo && isSizeInfoCompleted(sizeInfo) used twice. If those aren't true, maybe we should just early return or log an error here. It's fine to make assumptions/assertions about the data being present as long as we log/exit. Overly defensive programming like this just leads to weird edge cases
ae5692a to
1bcb819
Compare
| import type {UseApiQueryResult} from 'sentry/utils/queryClient'; | ||
| import type RequestError from 'sentry/utils/requestError/requestError'; | ||
| import {useQueryParamState} from 'sentry/utils/url/useQueryParamState'; | ||
| import {BuildDetailsMetricCards} from 'sentry/views/preprod/buildDetails/main/buildDetailsMetricCards'; |
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.
file name in commit has capital B (locally it's lower case B, not sure what's going on here)
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.
Not sure I'm following this, all looks fine to me in the file diff.
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.
Oh wait, I see the file change name, as long as everything renders locally with pnpm run dev-ui I think you should be fine
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.
Bug: Loading Skeleton Layout Mismatch
The loading skeleton doesn't match the new layout structure after adding BuildDetailsMetricCards. The skeleton shows controls at the top, but the actual rendered content has metric cards first, then controls/chart/legend, then insights. This causes a layout shift when loading completes, resulting in poor user experience.
static/app/views/preprod/buildDetails/main/buildDetailsMainContent.tsx#L119-L128
sentry/static/app/views/preprod/buildDetails/main/buildDetailsMainContent.tsx
Lines 119 to 128 in 0b9f967
| <Stack gap="lg" width="100%"> | |
| <Flex width="100%" justify="between" align="center" gap="md"> | |
| <Placeholder width="92px" height="40px" /> | |
| <Placeholder style={{flex: 1}} height="40px" /> | |
| </Flex> | |
| <Placeholder width="100%" height="540px" /> | |
| <Placeholder height="140px" /> | |
| </Stack> | |
| ); | |
| } |
| field: 'install_size_bytes' | 'download_size_bytes' | ||
| ): WatchBreakdown | undefined { | ||
| if (!primaryMetric || !watchMetric) { | ||
| return undefined; |
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.
No need to push a new commit but just for your reference, return; is the same as return undefined;. I usually prefer the former as it's one less thing to write.
| ${p => | ||
| p.$interactive | ||
| ? ` | ||
| text-decoration: underline dotted; |
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.
Not sure how important the cursor value is, but you can do underlined dotted text with <Text>: https://sentry.sentry.io/stories/typography/text#typographic-features
| label={card.title} | ||
| labelTooltip={card.labelTooltip} | ||
| action={ | ||
| card.showInsightsButton |
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: I usually like to extract things like this out to a variable in a block above the return() statement. Something like:
let metricAction;
if (card.showInsightsButton) {
metricAction = { ... }
}
then this just becomes
<MetricCard
...
action={metricAction}
>
add top level metrics to build details
Screen.Recording.2025-11-17.at.2.27.48.PM.mov
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.