fix(mobile): Lock body scroll when sidebar opens on mobile#17588
fix(mobile): Lock body scroll when sidebar opens on mobile#17588rahulchhabria wants to merge 1 commit intomasterfrom
Conversation
On iOS Safari, the mobile sidebar overlay wasn't scrollable because touch events were being captured by the scrollable body underneath. This adds body scroll locking when the sidebar opens on mobile, matching the existing behavior for the mobile search overlay. Slack thread: https://sentry.slack.com/archives/C8H0BQRDJ/p1777747105228359 https://claude.ai/code/session_01XpiMadpiudFwxMEpwPDpPE
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| const handleResize = () => { | ||
| if (window.innerWidth >= 768) { | ||
| document.body.style.overflow = ''; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Bug: The window resize handler removes the body scroll lock but doesn't update the sidebarOpen state, preventing the lock from being reapplied when resizing back to a mobile view.
Severity: LOW
Suggested Fix
In the resize event handler, call setSidebarOpen(false) when the window width is greater than or equal to 768px. This will ensure the state is correctly updated, allowing the useEffect hook to re-apply the scroll lock if the sidebar is opened again or if the user resizes back to a mobile view with the sidebar intended to be open.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/components/header.tsx#L133-L137
Potential issue: The resize handler for the sidebar scroll lock does not update the
`sidebarOpen` state. When a user on a mobile viewport opens the sidebar, resizes the
window to a desktop width (>=768px), and then resizes back to mobile, the scroll lock on
the body is not reapplied. This happens because the resize handler removes the scroll
lock but fails to set `sidebarOpen` to `false`, so the `useEffect` hook responsible for
applying the lock does not re-run when returning to the mobile view.
Did we get this right? 👍 / 👎 to inform future reviews.
| } | ||
| }; | ||
|
|
||
| window.addEventListener('resize', handleResize); | ||
| return () => { | ||
| document.body.style.overflow = ''; | ||
| window.removeEventListener('resize', handleResize); | ||
| }; | ||
| } | ||
| return undefined; | ||
| }, [sidebarOpen]); |
There was a problem hiding this comment.
Bug: The scroll lock effects for the sidebar and mobile search conflict. Closing one will remove the scroll lock even if the other component is still open and requires it.
Severity: MEDIUM
Suggested Fix
Refactor the scroll lock logic to be managed by a single, centralized mechanism. Use a counter or a shared state manager to track how many components are requesting a scroll lock. The lock should only be applied when the count is greater than zero and removed only when the count returns to zero.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/components/header.tsx#L128-L146
Potential issue: The `useEffect` hooks for the sidebar and the mobile search both manage
the body's scroll lock by directly manipulating `document.body.style.overflow`. If both
the sidebar and mobile search are opened, two separate effects will have applied a
scroll lock. However, when either one is closed, its cleanup function unconditionally
removes the scroll lock, regardless of whether the other component is still open and
requires it. This allows the body to scroll when it should be locked.
Did we get this right? 👍 / 👎 to inform future reviews.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3f9b279. Configure here.
| }; | ||
| } | ||
| return undefined; | ||
| }, [sidebarOpen]); |
There was a problem hiding this comment.
Competing overflow effects can cancel each other's scroll lock
Medium Severity
The new sidebarOpen effect and the existing mobileSearchOpen effect both independently manipulate document.body.style.overflow. When one effect cleans up, it unconditionally resets overflow to '', even if the other effect still requires it to be 'hidden'. On a non-home page on mobile, if the sidebar is open and the user opens then closes the search overlay, the search cleanup removes the sidebar's scroll lock — and vice versa.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 3f9b279. Configure here.
| if (window.innerWidth >= 768) { | ||
| document.body.style.overflow = ''; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Resize handler doesn't sync state unlike search effect
Low Severity
The sidebar resize handler directly sets document.body.style.overflow to '' at desktop width without updating sidebarOpen state. The analogous mobileSearchOpen resize handler correctly calls setMobileSearchOpen(false), which triggers a proper state change and cleanup. This inconsistency means if a user resizes from desktop back to mobile, sidebarOpen is still true but the effect won't re-run, so the scroll lock won't be re-applied.
Reviewed by Cursor Bugbot for commit 3f9b279. Configure here.
2 participants
On iOS Safari, the mobile sidebar overlay wasn't scrollable because
touch events were being captured by the scrollable body underneath.
This adds body scroll locking when the sidebar opens on mobile, matching
the existing behavior for the mobile search overlay.
Slack thread: https://sentry.slack.com/archives/C8H0BQRDJ/p1777747105228359
https://claude.ai/code/session_01XpiMadpiudFwxMEpwPDpPE