Skip to content

[CORRUPTED] Synthetic Benchmark PR #103469 - chore(preprod): abstract metric cards for build comparison #2

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
Open
tomerqodo wants to merge 3 commits into
base: base_pr_103469_20251204_2377
Choose a base branch
from
Open

[CORRUPTED] Synthetic Benchmark PR #103469 - chore(preprod): abstract metric cards for build comparison #2

tomerqodo wants to merge 3 commits into from

Conversation

@tomerqodo
Copy link

@tomerqodo tomerqodo commented Dec 4, 2025 •
edited by qodo-code-review bot
Loading

User description

Benchmark PR getsentry#103469

Type: Corrupted (contains bugs)

Original PR Title: chore(preprod): abstract metric cards for build comparison
Original PR Description: - small style changes

  • abstract card into component to reuse in follow up pr
image

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.

Original PR URL: getsentry#103469

PR Type

Enhancement

Description

  • Extract metric cards into reusable component for build comparison

  • Remove styled components dependency from main content file

  • Optimize metrics calculation with useMemo hook

  • Create new MetricCard component with flexible action support

Diagram Walkthrough

flowchart LR
  A["sizeCompareMainContent.tsx"] -- "extracts metrics logic" --> B["BuildComparisonMetricCards.tsx"]
  B -- "uses" --> C["MetricCard.tsx"]
  A -- "removes styled components" --> D["Inline styles"]
Loading

File Walkthrough
Relevant files
Enhancement
BuildComparisonMetricCards.tsx
New metric cards component with calculation logic               

static/app/views/preprod/buildComparison/main/BuildComparisonMetricCards.tsx

  • New component that encapsulates metric card rendering logic
  • Calculates install and download size metrics with percentage changes
  • Uses useMemo to optimize metric computation based on comparison data
  • Renders metrics using the new MetricCard component with trend
    indicators
 +168/-0 
sizeCompareMainContent.tsx
Refactor metrics rendering to external component                 

static/app/views/preprod/buildComparison/main/sizeCompareMainContent.tsx

  • Remove styled components import and DiffText styled component
  • Extract 68 lines of metric rendering logic to
    BuildComparisonMetricCards
  • Remove unused icon imports (IconArrow, IconCode, IconDownload)
  • Replace inline metrics grid with BuildComparisonMetricCards component
    call
  • Simplify component by delegating metric card rendering responsibility
 +6/-142 
metricCard.tsx
New reusable metric card component                                             

static/app/views/preprod/components/metricCard.tsx

  • New reusable MetricCard component for displaying metric information
  • Supports icon, label, optional tooltip, and action button
  • Provides flexible children prop for metric content rendering
  • Uses Stack and Flex layout components from scraps library
 +68/-0   

@qodo-code-review
Copy link

qodo-code-review bot commented Dec 4, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
⚪
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

⚪
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
No audit logging: The new UI components compute and display metrics but do not perform or invoke any audit
logging for critical actions, though it is unclear whether such logging is expected at
this presentation layer.

Referred Code 
export function BuildComparisonMetricCards(props: BuildComparisonMetricCardsProps) {
  const {comparisonResults, comparisonResponse} = props;

  const metrics = useMemo<ComparisonMetric[]>(() => {
    if (!comparisonResults) {
      return [];
    }

    const labels = getLabels(
      comparisonResponse?.head_build_details.app_info?.platform ?? undefined
    );
    const {size_metric_diff_item} = comparisonResults;

    return [
      {
        key: 'install',
        title: labels.installSizeLabel,
        icon: <IconCode size="sm" />,
        head: size_metric_diff_item.head_install_size,
        base: size_metric_diff_item.base_install_size,
        diff:


 ... (clipped 95 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Zero-division guard: Percentage change computation guards against zero base by returning 0, which may hide
meaningful state; no explicit empty-state UI is shown when metrics array is empty beyond
returning null upstream.

Referred Code 
  percentageChange:
    size_metric_diff_item.base_install_size === 0
      ? 0
      : (size_metric_diff_item.head_install_size -
          size_metric_diff_item.base_install_size) /
        size_metric_diff_item.base_install_size,
},
{
  key: 'download',
  title: labels.downloadSizeLabel,
  icon: <IconDownload size="sm" />,
  head: size_metric_diff_item.head_download_size,
  base: size_metric_diff_item.base_download_size,
  diff:
    size_metric_diff_item.head_download_size -
    size_metric_diff_item.base_download_size,
  percentageChange:
    size_metric_diff_item.base_download_size === 0
      ? 0
      : (size_metric_diff_item.head_download_size -
          size_metric_diff_item.base_download_size) /


 ... (clipped 2 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
External data use: The component renders values from comparison data without explicit validation or
sanitization in this layer, though these values appear numeric and may be validated
upstream.

Referred Code 
const metrics = useMemo<ComparisonMetric[]>(() => {
  if (!comparisonResults) {
    return [];
  }

  const labels = getLabels(
    comparisonResponse?.head_build_details.app_info?.platform ?? undefined
  );
  const {size_metric_diff_item} = comparisonResults;

  return [
    {
      key: 'install',
      title: labels.installSizeLabel,
      icon: <IconCode size="sm" />,
      head: size_metric_diff_item.head_install_size,
      base: size_metric_diff_item.base_install_size,
      diff:
        size_metric_diff_item.head_install_size -
        size_metric_diff_item.base_install_size,
      percentageChange:


 ... (clipped 24 lines)

Learn more about managing compliance generic rules or creating your own custom rules
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

qodo-code-review bot commented Dec 4, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Abstract the new trend utility

The new local getTrend helper function, which determines UI styles from a
numerical difference, should be abstracted into a shared utility to promote
reuse and consistency.

Examples:

static/app/views/preprod/buildComparison/main/BuildComparisonMetricCards.tsx [149-168] 
function getTrend(diff: number): {
  variant: 'danger' | 'success' | 'muted';
  icon?: React.ReactNode;
} {
  if (diff > 0) {
    return {
      variant: 'danger',
      icon: <IconArrow direction="up" size="xs" />,
    };
  }

 ... (clipped 10 lines)

Solution Walkthrough:

Before:

// In '.../BuildComparisonMetricCards.tsx'

export function BuildComparisonMetricCards(props) {
  // ...
  return (
    <Flex>
      {metrics.map(metric => {
        const {variant, icon} = getTrend(metric.diff);
        // ... render card using variant and icon
      })}
    </Flex>
  );
}

// Local helper function
function getTrend(diff: number) {
  if (diff > 0) {
    return { variant: 'danger', icon: <IconArrow direction="up" /> };
  }
  if (diff < 0) {
    return { variant: 'success', icon: <IconArrow direction="down" /> };
  }
  return { variant: 'muted' };
}

After:

// In a new file, e.g., 'sentry/utils/trends.ts'

export function getTrendIndicator(diff: number) {
  if (diff > 0) {
    return { variant: 'danger', icon: <IconArrow direction="up" /> };
  }
  if (diff < 0) {
    return { variant: 'success', icon: <IconArrow direction="down" /> };
  }
  return { variant: 'muted' };
}

// In '.../BuildComparisonMetricCards.tsx'
import {getTrendIndicator} from 'sentry/utils/trends';

export function BuildComparisonMetricCards(props) {
  // ...
  return (
    <Flex>
      {metrics.map(metric => {
        const {variant, icon} = getTrendIndicator(metric.diff);
        // ... render card using variant and icon
      })}
    </Flex>
  );
}
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies the new getTrend function as a good candidate for a shared utility, which would improve code reusability, though its immediate impact is moderate.

Low
General
Refactor metric calculations for clarity

Refactor the metric calculation within useMemo by creating a helper function to
compute diff and percentageChange, reducing code duplication.

static/app/views/preprod/buildComparison/main/BuildComparisonMetricCards.tsx [35-79]

 const metrics = useMemo<ComparisonMetric[]>(() => {
   if (!comparisonResults) {
     return [];
   }
+
+  const getMetricValues = (head: number, base: number) => {
+    const diff = head - base;
+    const percentageChange = base === 0 ? 0 : diff / base;
+    return {diff, percentageChange};
+  };
 
   const labels = getLabels(
     comparisonResponse?.head_build_details.app_info?.platform ?? undefined
   );
   const {size_metric_diff_item} = comparisonResults;
 
   return [
     {
       key: 'install',
       title: labels.installSizeLabel,
       icon: <IconCode size="sm" />,
       head: size_metric_diff_item.head_install_size,
       base: size_metric_diff_item.base_install_size,
-      diff:
-        size_metric_diff_item.head_install_size -
-        size_metric_diff_item.base_install_size,
-      percentageChange:
-        size_metric_diff_item.base_install_size === 0
-          ? 0
-          : (size_metric_diff_item.head_install_size -
-              size_metric_diff_item.base_install_size) /
-            size_metric_diff_item.base_install_size,
+      ...getMetricValues(
+        size_metric_diff_item.head_install_size,
+        size_metric_diff_item.base_install_size
+      ),
     },
     {
       key: 'download',
       title: labels.downloadSizeLabel,
       icon: <IconDownload size="sm" />,
       head: size_metric_diff_item.head_download_size,
       base: size_metric_diff_item.base_download_size,
-      diff:
-        size_metric_diff_item.head_download_size -
-        size_metric_diff_item.base_download_size,
-      percentageChange:
-        size_metric_diff_item.base_download_size === 0
-          ? 0
-          : (size_metric_diff_item.head_download_size -
-              size_metric_diff_item.base_download_size) /
-            size_metric_diff_item.base_download_size,
+      ...getMetricValues(
+        size_metric_diff_item.head_download_size,
+        size_metric_diff_item.base_download_size
+      ),
     },
   ];
 }, [comparisonResults, comparisonResponse]);
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies repeated logic and proposes a valid refactoring using a helper function to make the code more DRY and readable.

Low
Use more generic label text

Change the label text from Base Build Size: back to the more generic Comparison:
to better reflect that any two builds can be compared.

static/app/views/preprod/buildComparison/main/BuildComparisonMetricCards.tsx [134-139]

 <Text variant="muted" size="sm">
-  {t('Base Build Size:')}
+  {t('Comparison:')}
 </Text>
 <Text variant="muted" size="sm" bold>
   {metric.base === 0 ? t('Not present') : formatBytesBase10(metric.base)}
 </Text>
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly points out that the new label Base Build Size: is less generic than the original Comparison:, which was changed during the refactoring. Reverting this makes the UI text more accurate for arbitrary build comparisons.

Low
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Review effort 2/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tomerqodo @mtopo27