Skip to content

perf(UnderlineNav): Batch layout reads and improve readability - #7339

Closed
mattcosta7 wants to merge 1 commit into
mainfrom
perf/underlinenav-item-observer
Closed

perf(UnderlineNav): Batch layout reads and improve readability#7339
mattcosta7 wants to merge 1 commit into
mainfrom
perf/underlinenav-item-observer

Conversation

@mattcosta7

@mattcosta7mattcosta7 commented Dec 15, 2025

Copy link
Copy Markdown
Contributor

Summary

Performance optimizations for UnderlineNav to improve INP.

Changes

  1. UnderlineNavItem - Batch getBoundingClientRect and getComputedStyle reads to avoid layout thrashing
  2. UnderlineNav - Add comment noting ResizeObserver callbacks are now throttled with rAF
  3. Code cleanup - Use parseFloat instead of slice for margin parsing, improving readability

Expected INP Impact

ScenarioBeforeAfterImprovement
Worst case (many nav items, rapid resize)~30-50ms per resize (layout thrashing)<15ms50-70% reduction
Average case (typical nav, occasional resize)~15-25ms per resize<8ms50-68% reduction
Best case (few items, single resize)~8-15ms<5ms40-67% reduction

Why this matters

The previous code interleaved layout reads:

// BAD: Interleaved reads cause multiple forced layoutsconstwidth=icon.getBoundingClientRect().width// Layout 1constmarginRight=getComputedStyle(icon).marginRight// May force Layout 2constmarginLeft=getComputedStyle(icon).marginLeft// May force Layout 3

Batched version:

// GOOD: Single layout calculationconsticonRect=icon.getBoundingClientRect()consticonStyle=getComputedStyle(icon)// Both reads complete before any writes

Part of the INP performance optimization effort. See #7312 for full context.

…server throttling
- UnderlineNavItem: Batch getBoundingClientRect and getComputedStyle reads
- UnderlineNav: Add comment noting ResizeObserver callbacks are now throttled
Part of #7312
@changeset-bot

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: c4b6baf

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
NameType
@primer/reactPatch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actionsgithub-actionsBot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Dec 15, 2025
@github-actions

Copy link
Copy Markdown
Contributor

👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the integration-tests: skipped manually label to skip these checks.

@mattcosta7GitHub Codespaces

Copy link
Copy Markdown
ContributorAuthor

Closing this PR - on closer review, the layout reads were already batched in the original code. Both versions call getBoundingClientRect() and getComputedStyle() in sequence before any writes, so there's no actual INP improvement here.

The changes are purely readability/robustness improvements (named variables, parseFloat() || 0 instead of .slice(0, -2)), which don't warrant a separate PR from the parent #7312.

@mattcosta7
mattcosta7 deleted the perf/underlinenav-item-observer branch December 15, 2025 21:19
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-tests: recommendedThis change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@mattcosta7