fix(ui): Prevent body scroll when CompactSelect menu is open#108191
fix(ui): Prevent body scroll when CompactSelect menu is open#108191
Conversation
When a CompactSelect menu is open, the body overflow is now set to 'hidden' to prevent scroll propagation to the page. The previous overflow value is restored when the menu closes. This follows the same pattern used in the global drawer component to ensure proper overlay behavior and prevent background scrolling.
github-actions
bot
added
the
Scope: Frontend
| // Prevent body scroll while the select menu is open | ||
| previousOverflowRef.current ??= document.body.style.overflow; | ||
| document.body.style.overflow = 'hidden'; | ||
|
|
||
| // Force a overlay update, as sometimes the overlay is misaligned when opened |
There was a problem hiding this comment.
@TkDodo there are now two places where we use this, so it may be worth consolidating if we have another instance of it
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Addresses PR feedback by creating a shared utility that coordinates body overflow locking across CompactSelect and GlobalDrawer components. Key improvements: - Prevents race conditions when multiple overlays are active simultaneously - Adds cleanup on CompactSelect unmount to prevent permanent scroll lock - Uses reference counting to ensure overflow is only restored when all locks are released - Eliminates code duplication between components Fixes issues where body scroll could remain locked if: 1. CompactSelect unmounted while menu was open 2. Multiple components closed in unexpected order
Wrap the return object in useMemo to prevent unnecessary re-renders in components that use this hook.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
|
|
||
| // On close | ||
| onClose?.(); | ||
| scrollLock.release(); |
There was a problem hiding this comment.
Deferred scroll lock acquire can orphan permanent body lock
Medium Severity
scrollLock.acquire() and scrollLock.release() are called inside nextFrameCallback (a requestAnimationFrame wrapper), but the useScrollLock cleanup releases synchronously on unmount. If the component unmounts between the rAF being scheduled and firing (e.g., the onOpenChange prop callback triggers a parent state change that removes the CompactSelect), the deferred acquire will execute after cleanup, setting document.body.style.overflow to 'hidden' on an orphaned Lock instance with no future cleanup — making the page permanently unscrollable. Unlike the GlobalDrawer usage, which calls acquire/release synchronously, this deferred path has no cancellation guard.
Additional Locations (1)
There was a problem hiding this comment.
Ok, this is legit, but can also be a problem for other calls
@cursoragent store the raf id in a react ref, and cancel the raf from the cleanup callback
|
|
||
| if (this.acquiredBy.size === 0 && this.initialOverflow !== null) { | ||
| this.container.style.overflow = this.initialOverflow; | ||
| if (this.container === document.body) { |
There was a problem hiding this comment.
We update the scrollbar gutter setting so that we don't get layout shift. We do it conditionally so that in the views without scrollbars, there is no reserved white area where the scrollbar would otherwise be
TkDodo
left a comment
TkDodo
left a comment
There was a problem hiding this comment.
approving, but please move the util to scraps before merging
Moves useScrollLock utility and its test file from static/app/utils/ to static/app/components/core/ to better organize core UI utilities alongside other core components.
|
I'm holding off on merging until I figure out if we want this to have a proper backdrop component or not so that we can also prevent clicks. Right now it is quite wonky in the sense that hovering other components displays their interaction states, but then we also block the page. I'm not happy sold on this being the intermediary state |
2 participants
When a CompactSelect menu is open, the body overflow is now set to 'hidden' to prevent scroll propagation to the page. The previous overflow value is restored when the menu closes.
This follows the same pattern used in the global drawer component to ensure proper overlay behavior and prevent background scrolling.