Skip to content

Conversation

@tomerqodo
Copy link

@tomerqodo tomerqodo commented Dec 4, 2025

User description

Benchmark PR getsentry#102980

Type: Clean (correct implementation)

Original PR Title: breadcrumbs: remove styled and simplify
Original PR Description: Simplify breadcrumb component and add amplitude logging.


PR Type

Enhancement


Description

  • Simplify breadcrumbs component by removing styled-components and using layout/text components

  • Remove optional key prop and linkLastItem prop for cleaner API

  • Add amplitude analytics tracking for breadcrumb interactions

  • Improve type safety by requiring non-null labels in Crumb interface

  • Update all breadcrumb usages to remove deprecated props and add analytics


Diagram Walkthrough

flowchart LR
  A["Breadcrumbs Component"] -->|Remove styled| B["Use Scraps Layout/Text"]
  A -->|Simplify Props| C["Remove key and linkLastItem"]
  A -->|Add Analytics| D["Track link clicks and menu opens"]
  E["Chevron Component"] -->|Replaced| F["IconChevron from icons"]
  G["Breadcrumb Usages"] -->|Update| H["Remove deprecated props"]
Loading

File Walkthrough

Relevant files
Enhancement
11 files
breadcrumbs.tsx
Refactor breadcrumbs to use layout components and add analytics
+73/-90 
chevron.tsx
Remove custom Chevron component implementation                     
+0/-90   
analytics.tsx
Register breadcrumbs analytics events in event map             
+4/-0     
breadcrumbsAnalyticsEvents.tsx
Create new breadcrumbs analytics events definitions           
+12/-0   
landing.tsx
Remove deprecated key props from breadcrumbs                         
+0/-2     
buildCompareHeaderContent.tsx
Add translation and null-safety to breadcrumb labels         
+3/-3     
replayDetailsPageBreadcrumbs.tsx
Add fallback translation for null project label                   
+3/-1     
index.tsx
Add analytics tracking to settings breadcrumb links           
+9/-1     
organizationCrumb.tsx
Track analytics when organization breadcrumb menu opens   
+6/-0     
projectCrumb.tsx
Track analytics when project breadcrumb menu opens             
+6/-0     
teamCrumb.tsx
Track analytics when team breadcrumb menu opens                   
+6/-0     
Documentation
1 files
breadcrumbs.stories.tsx
Update breadcrumbs stories to reflect simplified API         
+8/-22   
Tests
2 files
landing.spec.tsx
Update test to use getByRole instead of getByText               
+1/-1     
results.spec.tsx
Update test to use getByRole instead of getByText               
+1/-1     

@qodo-code-review
Copy link

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: Security-First Input Validation and Data Handling

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

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:
Limited context: New analytics events for breadcrumb interactions are tracked but include only an
organization field set to null, lacking user and outcome context required for
comprehensive audit trails.

Referred Code
function onBreadcrumbLinkClick() {
  if (props.crumb.to) {
    trackAnalytics('breadcrumbs.link.clicked', {organization: null});
  }
}

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:
Missing guards: The click handler and rendering assume valid crumb targets and labels without explicit
error handling or validation in new code, which may miss edge cases if external inputs are
malformed.

Referred Code
function BreadCrumbItem(props: BreadCrumbItemProps) {
  function onBreadcrumbLinkClick() {
    if (props.crumb.to) {
      trackAnalytics('breadcrumbs.link.clicked', {organization: null});
    }
  }

  return (
    <Container maxWidth="400px" width="auto">
      {styleProps => {
        return props.crumb.to ? (
          <BreadcrumbLink
            to={props.crumb.to}
            preservePageFilters={props.crumb.preservePageFilters}
            data-test-id="breadcrumb-link"
            onClick={onBreadcrumbLinkClick}
            {...styleProps}
          >
            <Text ellipsis variant={props.variant}>
              {props.crumb.label}
            </Text>


 ... (clipped 15 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

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Consider unifying breadcrumb analytics implementation

To reduce code duplication, centralize the analytics logic for breadcrumb
interactions. Create a shared utility or hook for tracking events like clicks
and menu openings, ensuring consistent behavior across different breadcrumb
components.

Examples:

static/app/views/settings/components/settingsBreadcrumb/organizationCrumb.tsx [80-84]
      onOpenChange={open => {
        if (open) {
          trackAnalytics('breadcrumbs.menu.opened', {organization: null});
        }
      }}
static/app/views/settings/components/settingsBreadcrumb/projectCrumb.tsx [69-73]
      onOpenChange={open => {
        if (open) {
          trackAnalytics('breadcrumbs.menu.opened', {organization: null});
        }
      }}

Solution Walkthrough:

Before:

// In static/app/views/settings/components/settingsBreadcrumb/organizationCrumb.tsx
function OrganizationCrumb(...) {
  return (
    <BreadcrumbDropdown
      onOpenChange={open => {
        if (open) {
          trackAnalytics('breadcrumbs.menu.opened', {organization: null});
        }
      }}
      ...
    />
  );
}

// In static/app/views/settings/components/settingsBreadcrumb/index.tsx
function SettingsBreadcrumb(...) {
  function onSettingsBreadcrumbLinkClick() {
    trackAnalytics('breadcrumbs.link.clicked', {organization: null});
  }
  ...
  <CrumbLink onClick={onSettingsBreadcrumbLinkClick} ... />
}

After:

// In a new file, e.g., 'sentry/utils/analytics/useBreadcrumbAnalytics.ts'
function useBreadcrumbAnalytics() {
  const onLinkClick = useCallback(() => {
    trackAnalytics('breadcrumbs.link.clicked', {organization: null});
  }, []);

  const onMenuOpen = useCallback(() => {
    trackAnalytics('breadcrumbs.menu.opened', {organization: null});
  }, []);

  return { onLinkClick, onMenuOpen };
}

// In static/app/views/settings/components/settingsBreadcrumb/organizationCrumb.tsx
function OrganizationCrumb(...) {
  const { onMenuOpen } = useBreadcrumbAnalytics();
  return (
    <BreadcrumbDropdown onOpenChange={open => open && onMenuOpen()} ... />
  );
}
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies duplicated analytics tracking logic for breadcrumb interactions across multiple components and proposes a valid solution to centralize it, which would improve code maintainability and consistency.

Low
General
Remove redundant truthiness check

In onBreadcrumbLinkClick, remove the redundant if (props.crumb.to) check, as it
is only called when props.crumb.to is already confirmed to be truthy.

static/app/components/breadcrumbs.tsx [76-80]

 function onBreadcrumbLinkClick() {
-  if (props.crumb.to) {
-    trackAnalytics('breadcrumbs.link.clicked', {organization: null});
-  }
+  trackAnalytics('breadcrumbs.link.clicked', {organization: null});
 }
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies a redundant if check, as the function is only called in a context where the condition is always true, simplifying the code.

Low
  • More

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants