perf(useAnchoredPosition): cache scrollable-ancestor walk per anchor#8116
perf(useAnchoredPosition): cache scrollable-ancestor walk per anchor#8116mattcosta7 wants to merge 9 commits into
Conversation
The scroll-listener setup effect lists `updatePosition` in its deps, so it re-walked the entire ancestor chain (a getComputedStyle per ancestor) every time a consumer's reposition dependencies changed — even though the set of scrollable ancestors only depends on the anchor element. Cache the walk keyed on the anchor element and reuse it when the anchor is unchanged (the common case). Listener attach/detach behavior is unchanged; only the redundant getComputedStyle walk is skipped.
🦋 Changeset detectedLatest commit: 1c3d9b9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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-actions
Bot
added
the
integration-tests: recommended
|
`handleScroll` only schedules a rAF and never calls preventDefault, so the scroll listeners attached to the anchor's scrollable ancestors can be passive. This lets Safari/Chrome scroll without waiting to see if the default is cancelled, improving scroll smoothness for overlays, menus, and tooltips.
mattcosta7
changed the title
…passive is a no-op)
mattcosta7
changed the title
There was a problem hiding this comment.
Pull request overview
This PR optimizes useAnchoredPosition by memoizing the scrollable-ancestor traversal (which calls getComputedStyle up the ancestor chain) so the scroll-listener effect can re-run on updatePosition changes without repeating the DOM walk when the anchor is unchanged.
Changes:
- Cache the result of
getScrollableAncestors(anchorEl)across effect re-runs keyed by anchor identity. - Add a patch changeset documenting the performance improvement for overlays/menus/tooltips.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/hooks/useAnchoredPosition.ts | Adds a ref-backed cache to avoid repeated scrollable-ancestor traversal when the scroll-listener effect re-runs. |
| .changeset/perf-anchored-position-cache-scrollables.md | Patch changeset describing the useAnchoredPosition perf improvement. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Low
mattcosta7
added
the
Canary Release
|
Integration test results from github/github-ui PR: |
these all passed, but aren't reporting right? |
mattcosta7
added
the
integration-tests: passing
…n-cache-scrollables
|
siddharthkp
left a comment
siddharthkp
left a comment
There was a problem hiding this comment.
Trust you to thoroughly test these!
yeah github-ui sometimes does that, I'm not sure why: https://github.com/github/github-ui/pull/25847#issuecomment-4906163549 |
Closes #
useAnchoredPosition(used by every Overlay, AnchoredOverlay, menu, tooltip, and popover) attaches scroll listeners to the anchor's scrollable ancestors. The setup effect listsupdatePositionin its dependencies, so it re-walked the entire ancestor chain — callinggetComputedStyleon every ancestor — each time a consumer's reposition dependencies changed, even though the set of scrollable ancestors only depends on the anchor element.This caches the walk keyed on the anchor element and reuses it when the anchor is unchanged (the common case). Listener attach/detach behavior is unchanged; only the redundant
getComputedStylewalk is skipped.Changelog
Changed
useAnchoredPositioncaches its scrollable-ancestor walk per anchor element, avoiding repeatedgetComputedStyleancestor traversals (and the style recalc they force) when the reposition dependencies change while an overlay is open.Performance
Each avoided walk (a
getComputedStyleper ancestor) costs roughly, at 1× CPU:The win scales with DOM depth and with how often the overlay's consumer changes reposition dependencies while open; more importantly it removes a forced style recalc from that path during interaction.
Rollout strategy
Testing & Reviewing
useAnchoredPosition,Overlay, andAnchoredOverlaysuites; eslint/prettier clean.overflowstyle changed while the same overlay stayed open, the cached ancestor list could be momentarily stale (the previous code only caught this incidentally too). This is a high-blast-radius core hook — please run the github/github-ui integration suite to confirm overlay/menu/tooltip repositioning on scroll is unaffected.Merge checklist