Optimize useAnchoredPosition hook for improved INP and rendering performance #7362
Conversation
🦋 Changeset detectedLatest commit: d331c1a 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 |
|
👋 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 |
github-actions
bot
added
the
integration-tests: recommended
There was a problem hiding this comment.
Pull request overview
This PR optimizes the useAnchoredPosition hook to improve rendering performance and reduce layout thrashing for components using anchored positioning (ActionMenu, SelectPanel, Overlay, etc.). The optimization focuses on batching DOM reads/writes, using requestAnimationFrame for update coalescing, and minimizing unnecessary React re-renders through ref-based mutable state.
Key changes:
- Replaces state-based tracking with ref-based mutable state for values that don't need to trigger re-renders (previous position, height, pending status)
- Implements requestAnimationFrame batching to coalesce multiple update triggers into single calculations
- Replaces
useResizeObserverhook with direct ResizeObserver usage for more granular control over both anchor and floating elements - Deprecates the
dependenciesparameter (now ignored but kept for backwards compatibility)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| packages/react/src/hooks/useAnchoredPosition.ts | Refactored hook implementation with RAF batching, ref-based state, direct ResizeObserver usage, and position equality checks to avoid unnecessary re-renders |
| packages/react/src/hooks/tests/useAnchoredPosition.test.tsx | Added comprehensive test suite with 18 tests covering basic functionality, provided refs, callbacks, settings, resize handling, cleanup, multiple instances, and edge cases |
packages/react/src/hooks/__tests__/useAnchoredPosition.test.tsx
Outdated
Show resolved
Hide resolved
- Add ResizeObserver environment check with fallback to window resize - Fix pin position logic to update prevHeight after pinning - Update dependencies test to reflect deprecated parameter behavior - Add comprehensive ResizeObserver tests - Add comments explaining state machine and design decisions - Only create ResizeObserver when both elements are present - Add changeset for patch release
mattcosta7
changed the title
| export function useAnchoredPosition( | ||
| settings?: AnchoredPositionHookSettings, | ||
| dependencies: React.DependencyList = [], | ||
| _dependencies?: React.DependencyList, |
There was a problem hiding this comment.
leaving this, but deprecated to avoid breaking consumers - we handle this internally, so we shouldn't need to also handle it externally
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/9372 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (2)
packages/react/src/hooks/tests/useAnchoredPosition.test.tsx:335
- The test comment claims "onPositionChange is called even if position hasn't changed to notify consumers of potential layout changes", but this contradicts the actual implementation. The implementation only calls onPositionChange when position values actually change (lines 129-139 in useAnchoredPosition.ts). Either the comment is incorrect or the test expectation doesn't match the intended behavior.
// onPositionChange is called even if position hasn't changed
// to notify consumers of potential layout changes
expect(onPositionChange.mock.calls.length).toBeGreaterThanOrEqual(callCountBeforeResize)
packages/react/src/hooks/useAnchoredPosition.ts:68
- The settingsRef is updated in a useLayoutEffect (lines 66-68) without dependencies, which means it runs on every render. This is correct for keeping the ref in sync, but the pattern could be clearer. Consider adding a comment explaining that this must run on every render to ensure settingsRef.current always has the latest settings for async callbacks from requestAnimationFrame.
const settingsRef = React.useRef(settings)
useLayoutEffect(() => {
settingsRef.current = settings
})
| ) | ||
| } | ||
|
|
||
| describe('useAnchoredPosition', () => { |
There was a problem hiding this comment.
The pinPosition feature logic has been refactored but lacks test coverage. The pin logic prevents visual jumping when an anchored element is at the top and shrinking, but there are no tests validating this behavior works correctly after the refactoring. Consider adding tests that verify the pinPosition logic, especially since this feature is used in production components like SelectPanel.
This issue also appears in the following locations of the same file:
- line 333
| // Only update state if position actually changed to avoid unnecessary re-renders | ||
| if ( | ||
| !prevPosition || | ||
| prevPosition.top !== newPosition.top || | ||
| prevPosition.left !== newPosition.left || | ||
| prevPosition.anchorSide !== newPosition.anchorSide || | ||
| prevPosition.anchorAlign !== newPosition.anchorAlign | ||
| ) { | ||
| state.prevPosition = newPosition | ||
| setPosition(newPosition) | ||
| settingsRef.current?.onPositionChange?.(newPosition) |
There was a problem hiding this comment.
This is a breaking behavior change. The old implementation always updated position state with newPosition (unless pinned) and called onPositionChange when anchorSide matched. The new implementation only updates state and calls onPositionChange when position values actually differ. This means:
- Old: State always updates (even if values are same), onPositionChange called when anchorSide matches
- New: State only updates when values differ, onPositionChange only called when values differ
This could break consumers who rely on position state updates or onPositionChange callbacks being fired on every recalculation. For example, if a component uses position state as a render dependency to trigger other effects, those effects won't fire when position values happen to be the same. This optimization should be clearly documented as a potentially breaking change.
| // Only update state if position actually changed to avoid unnecessary re-renders | |
| if ( | |
| !prevPosition || | |
| prevPosition.top !== newPosition.top || | |
| prevPosition.left !== newPosition.left || | |
| prevPosition.anchorSide !== newPosition.anchorSide || | |
| prevPosition.anchorAlign !== newPosition.anchorAlign | |
| ) { | |
| state.prevPosition = newPosition | |
| setPosition(newPosition) | |
| settingsRef.current?.onPositionChange?.(newPosition) | |
| // Always update state with the new position (unless pinned) | |
| state.prevPosition = newPosition | |
| setPosition(newPosition) | |
| const configuredAnchorSide = currentSettings?.anchorSide | |
| if (!configuredAnchorSide || configuredAnchorSide === newPosition.anchorSide) { | |
| currentSettings?.onPositionChange?.(newPosition) |
| // Sync refs to state on every render. The setState calls are guarded to only | ||
| // trigger re-renders when the ref values actually change. | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps -- intentionally runs every render to detect ref changes | ||
| useLayoutEffect(() => { | ||
| const floatingCurrent = floatingElementRef.current | ||
| const anchorCurrent = anchorElementRef.current | ||
| if (floatingCurrent !== floatingEl) setFloatingEl(floatingCurrent) | ||
| if (anchorCurrent !== anchorEl) setAnchorEl(anchorCurrent) | ||
| }) |
There was a problem hiding this comment.
The useLayoutEffect that syncs refs to state (lines 73-78) runs on every render without dependencies, which is necessary to detect ref changes. However, the comment on line 72 says "intentionally runs every render" but this could cause performance issues in some scenarios. While the setState calls are guarded, the effect still runs its conditional logic on every render. Consider documenting why this pattern is necessary over alternatives like using a ref callback or useSyncExternalStore.
This issue also appears in the following locations of the same file:
- line 65
| useLayoutEffect(() => { | ||
| if (!floatingEl || !anchorEl) return | ||
| if (!hasResizeObserver) return // Fall back to window resize only |
There was a problem hiding this comment.
When both elements are present but ResizeObserver is not available (line 162), the hook falls back to window resize events only. However, this means element-specific size changes (like content changes that affect height) won't trigger position recalculation in environments without ResizeObserver. Consider documenting this limitation more clearly in the function's JSDoc, as it could lead to positioning bugs in older browsers.
| // Trigger resize observer callbacks for all instances observing the given element | ||
| function triggerResizeObserver(element?: Element) { | ||
| for (const instance of resizeObserverInstances) { | ||
| if (!element || instance.observedElements.has(element)) { |
There was a problem hiding this comment.
This negation always evaluates to true.
Overview
This PR optimizes the
useAnchoredPositionhook to improve Interaction to Next Paint (INP) and overall rendering performance for components that use anchored positioning (ActionMenu, SelectPanel, Overlay, Autocomplete, etc.).Key Performance Improvements
1. Reduced Layout Thrashing
clientHeight,getBoundingClientRect) before any writesrequestAnimationFrameto coalesce multiple update triggers into a single calculation2. Optimized React Re-renders
3. Smarter Element Observation
useResizeObserverhook for more granular control4. Improved Late-Mounting Element Handling
useLayoutEffectExpected Web Vitals Improvements
INP (Interaction to Next Paint)
Opening/closing overlays with complex DOM
TBT (Total Blocking Time)
Rapid resize events or window resizing
CLS (Cumulative Layout Shift)
Position recalculations during page load
FID (First Input Delay)
Initial interaction with overlay triggers
How These Improvements Are Achieved
Impact by Usage Pattern
Key Optimizations
Why These Improvements?
INP (Interaction to Next Paint):
TBT (Total Blocking Time):
CLS (Cumulative Layout Shift):
API Changes
Changed
dependenciesparameter is now deprecated and ignored. Position updates are handled automatically via ResizeObserver and window resize events. The parameter is kept for backwards compatibility but has no effect.Changelog
New
useAnchoredPositionhook (18 tests covering basic functionality, provided refs, callbacks, settings, resize handling, cleanup, edge cases, and more)Changed
useAnchoredPositionnow automatically detects element size changes via ResizeObserverrequestAnimationFrameto prevent layout thrashingdependenciesparameter is deprecated (still accepted for backwards compatibility)Removed
useResizeObserverhook (now uses ResizeObserver directly for more control)Rollout strategy
This is a performance optimization with no breaking API changes. The
dependenciesparameter is deprecated but still accepted.Testing & Reviewing
npm test -- --run packages/react/src/hooks/__tests__/useAnchoredPosition.test.tsxMerge checklist