fix: prevent background scroll when roadmap modal is open #4710
Conversation
- Added overflow hidden to body when modal opens to lock background scroll - Added cleanup function to restore scroll when modal closes - Fixes scroll chaining issue where background scrolls after reaching modal bottom Fixes asyncapi#4684
sammy200-ui
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg,
princerajpoot20 and
sambhavgupta0705
as code owners
WalkthroughThe Modal component now prevents background page scrolling when mounted by setting Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4710 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 799 799
Branches 146 146
=========================================
Hits 799 799 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #4684 by preventing background scroll when the roadmap modal is open. The implementation adds an overflow lock to the document body when the Modal component mounts and restores it on unmount using React's useEffect cleanup function.
Key Changes:
- Added
document.body.style.overflow = 'hidden'in the Modal component's useEffect to lock background scroll when modal opens - Added cleanup function to restore scroll by setting
document.body.style.overflow = ''when modal closes
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| document.body.style.overflow = 'hidden'; | ||
|
|
||
| // Cleanup: restore scroll when modal is closed | ||
| return () => { | ||
| document.body.style.overflow = ''; |
There was a problem hiding this comment.
The current implementation doesn't handle the case where multiple Modal instances might be mounted simultaneously. If two modals are open and the first one closes, it will restore body scroll even though the second modal is still open. Consider implementing a reference counting mechanism or using a shared state manager to track the number of open modals, and only restore overflow when the count reaches zero.
| document.body.style.overflow = 'hidden'; | |
| // Cleanup: restore scroll when modal is closed | |
| return () => { | |
| document.body.style.overflow = ''; | |
| openModalCount += 1; | |
| if (openModalCount === 1) { | |
| document.body.style.overflow = 'hidden'; | |
| } | |
| // Cleanup: restore scroll when modal is closed | |
| return () => { | |
| openModalCount -= 1; | |
| if (openModalCount === 0) { | |
| document.body.style.overflow = ''; | |
| } |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4710--asyncapi-website.netlify.app/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/Modal.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
- GitHub Check: cypress-run
- GitHub Check: Lighthouse CI
- GitHub Check: Test NodeJS PR - windows-latest
| // Focus the modal when it mounts and lock body scroll | ||
| useEffect(() => { | ||
| if (modalRef.current) modalRef.current.focus(); | ||
|
|
||
| // Prevent background scroll when modal is open | ||
| document.body.style.overflow = 'hidden'; | ||
|
|
||
| // Cleanup: restore scroll when modal is closed | ||
| return () => { | ||
| document.body.style.overflow = ''; | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the Modal component file structure
find . -type f -name "Modal.tsx" | head -5Repository: asyncapi/website
Length of output: 82
🏁 Script executed:
# Check for all references to body.style.overflow in the codebase
rg "body\.style\.overflow|document\.body\.style\.overflow" --type tsx --type ts -nRepository: asyncapi/website
Length of output: 87
🏁 Script executed:
# Search for Modal usage patterns to understand stacking scenarios
rg "Modal|<Modal" --type tsx --type ts -B 2 -A 2 | head -100Repository: asyncapi/website
Length of output: 87
🏁 Script executed:
# Check for overscroll-behavior in CSS or Tailwind configs
rg "overscroll|scroll-contain" --type css --type tsx -nRepository: asyncapi/website
Length of output: 87
🏁 Script executed:
# Look at the complete Modal component to understand context
cat -n components/Modal.tsx 2>/dev/null || find . -path "*/components/Modal.tsx" -exec cat -n {} \;Repository: asyncapi/website
Length of output: 3256
🏁 Script executed:
# Search for Modal imports and usage
grep -r "Modal" --include="*.tsx" --include="*.ts" --include="*.jsx" --include="*.js" | grep -i "import\|<Modal" | head -20Repository: asyncapi/website
Length of output: 326
🏁 Script executed:
# Search for body.style.overflow without file type restriction
grep -r "body\.style\.overflow\|document\.body\.style\.overflow" . 2>/dev/null | grep -v node_modulesRepository: asyncapi/website
Length of output: 191
🏁 Script executed:
# Check if there are multiple Modal instances rendered simultaneously anywhere
grep -r "<Modal" --include="*.tsx" --include="*.jsx" -B 5 -A 5 | head -60Repository: asyncapi/website
Length of output: 721
🏁 Script executed:
# Look for any overflow manipulation in CSS or global styles
find . -name "*.css" -o -name "tailwind.config.*" | xargs grep -l "overflow\|overscroll" 2>/dev/null | head -10Repository: asyncapi/website
Length of output: 101
🏁 Script executed:
# Check globals.css for any overflow or body styles
cat -n ./styles/globals.css | head -100Repository: asyncapi/website
Length of output: 2501
🏁 Script executed:
# Check tailwind.config.ts for overscroll or scroll-related settings
cat -n ./tailwind.config.tsRepository: asyncapi/website
Length of output: 7484
Restore previous body.style.overflow as defensive programming for potential modal stacking.
The current cleanup sets document.body.style.overflow = '', which could cause issues if (a) the modal component is reused in contexts where multiple modals stack, or (b) body.style.overflow is set elsewhere in the future. While the current codebase only renders a single Modal instance at a time (in RoadmapPill.tsx), storing and restoring the previous value follows best practices and prevents regressions:
export default function Modal({ title, children, onModalClose = () => {} }: IModalProps) {
const modalRef = useRef<HTMLDivElement>(null);
+ const prevBodyOverflowRef = useRef<string>('');
// Focus the modal when it mounts and lock body scroll
useEffect(() => {
if (modalRef.current) modalRef.current.focus();
// Prevent background scroll when modal is open
- document.body.style.overflow = 'hidden';
+ prevBodyOverflowRef.current = document.body.style.overflow;
+ document.body.style.overflow = 'hidden';
// Cleanup: restore scroll when modal is closed
return () => {
- document.body.style.overflow = '';
+ document.body.style.overflow = prevBodyOverflowRef.current;
};
}, []);The optional overscroll-behavior: contain mitigation is not currently needed, as the modal uses fixed positioning and doesn't exhibit scroll chaining behavior.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In components/Modal.tsx around lines 18–29, the effect currently sets
document.body.style.overflow = 'hidden' and restores it to '' on cleanup; change
it to capture the previous document.body.style.overflow value before
overwriting, set overflow to 'hidden' on mount, and restore the saved previous
value on cleanup so stacked modals or external overflow modifications are
preserved (save value to a local variable outside the cleanup and assign it back
in the return function).
|
@princerajpoot20 Modal is only used in the suggestion left by copilot and coderabbit is unnecessary for the current implementation. |
2 participants
Fixes #4684
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.