fix(livechat): reset smallScreen state when leaving mobile layout#38764
fix(livechat): reset smallScreen state when leaving mobile layout#38764Shreyas2004wagh wants to merge 1 commit intoRocketChat:developfrom
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughFixes the Livechat widget's media query handler to explicitly reset the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/meteor/packages/rocketchat-livechat/assets/rocket-livechat.js (2)
724-736:⚠️ Potential issue | 🟠 MajorBody scroll-lock left applied when widget is open during mobile→desktop resize
The one-line fix is correct for the primary described scenario (widget closed on mobile → resize → open on desktop). However, it introduces a regression for the case where the widget is already open in mobile mode when the resize occurs:
openWidgetis called whilesmallScreen === true→bodyStyleis saved andoverflow:hidden; position:fixedis applied todocument.body.style.cssText.- User resizes to desktop →
mediaqueryresponsefires and setssmallScreen = false.- User closes the widget →
closeWidgetevaluatesif (smallScreen)→false→ skipsdocument.body.style.cssText = bodyStyle→ body scroll-lock is never cleared.Before this PR, step 3 would at least restore
bodyStyle(becausesmallScreenwas stilltrueafter resize), so the close path worked correctly even if the subsequent open was broken. The fix removes that accidental safety net without replacing it.🐛 Proposed fix: restore body styles in `mediaqueryresponse` when transitioning while open
} else { + if (smallScreen && widget.dataset.state === 'opened') { + document.body.style.cssText = bodyStyle; + document.body.scrollTop = scrollPosition; + } smallScreen = false; chatWidget.style.left = 'auto'; chatWidget.style.right = '50px'; chatWidget.style.width = widgetWidth; }
widget,bodyStyle, andscrollPositionare all in scope via the enclosinginitclosure, so no additional wiring is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/packages/rocketchat-livechat/assets/rocket-livechat.js` around lines 724 - 736, mediaqueryresponse currently flips smallScreen and widget positioning but doesn't clear the mobile body scroll-lock when transitioning to desktop while the widget is open; update mediaqueryresponse so that when mql.matches is false and the widget is currently open (check the same flag/condition openWidget uses), restore document.body.style.cssText from the saved bodyStyle and restore scroll via scrollPosition before setting smallScreen = false and applying desktop styles to chatWidget; reference mediaqueryresponse, smallScreen, openWidget, closeWidget, bodyStyle, scrollPosition, and chatWidget to locate and implement this restoration.
724-736:⚠️ Potential issue | 🟠 MajorBody scroll-lock persists when the widget is open during a mobile→desktop resize
The one-line fix correctly handles the primary scenario (widget closed on mobile → resize → open on desktop). However, it introduces a regression when the widget is already open in mobile mode during the resize:
openWidgetruns withsmallScreen === true→ savesbodyStyleand appliesoverflow:hidden; position:fixedtodocument.body.- User resizes to desktop →
mediaqueryresponsefires →smallScreen = false(the new line).- User closes the widget →
closeWidgetchecksif (smallScreen)→false→ skipsdocument.body.style.cssText = bodyStyle→ the body scroll-lock is never cleared.Before this PR, step 3 incidentally worked correctly (body was restored), because
smallScreenwas stilltrueduring the close. This PR removes that accidental safety net without replacing it.🐛 Proposed fix: restore body styles on desktop transition when widget is open
} else { + if (smallScreen && widget.dataset.state === 'opened') { + document.body.style.cssText = bodyStyle; + document.body.scrollTop = scrollPosition; + } smallScreen = false; chatWidget.style.left = 'auto'; chatWidget.style.right = '50px'; chatWidget.style.width = widgetWidth; }
widget,bodyStyle, andscrollPositionare all accessible in themediaqueryresponseclosure (declared in the enclosinginitscope), so no additional wiring is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/packages/rocketchat-livechat/assets/rocket-livechat.js` around lines 724 - 736, mediaqueryresponse currently flips smallScreen on desktop but doesn't undo the mobile body scroll-lock if the widget is open; update mediaqueryresponse so that when switching from mobile to desktop (mql.matches false) and the widget is open (check the widget's open state), restore document.body.style.cssText = bodyStyle and restore scroll position (using scrollPosition) before setting smallScreen = false; reference the mediaqueryresponse closure variables widget, bodyStyle, scrollPosition, smallScreen, and ensure this mirrors the cleanup done by closeWidget so the body lock is not left active after a resize.
🧹 Nitpick comments (2)
apps/meteor/packages/rocketchat-livechat/assets/rocket-livechat.js (2)
738-740: Pre-existing deprecatedaddListenerAPI (good opportunity to modernise)
MediaQueryList.addListener()is deprecated, and you should instead useaddEventListener()to watch for thechangeevent. Since this code is already being touched, it's a low-risk opportunity to switch. Note that for Safari versions prior to 14,addEventListeneronMediaQueryListthrows aTypeError, so a feature-detect guard is advisable if older Safari support is required.♻️ Proposed modernisation
- var mql = window.matchMedia('screen and (max-device-width: 480px)'); + var mql = window.matchMedia('screen and (max-width: 480px)'); mediaqueryresponse(mql); - mql.addListener(mediaqueryresponse); + if (mql.addEventListener) { + mql.addEventListener('change', mediaqueryresponse); + } else { + mql.addListener(mediaqueryresponse); + }
max-device-width(physical device resolution) is also deprecated in Media Queries Level 4 —max-width(viewport width) is the modern and more semantically appropriate replacement for a responsive-layout breakpoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/packages/rocketchat-livechat/assets/rocket-livechat.js` around lines 738 - 740, The code uses the deprecated MediaQueryList.addListener via mql.addListener(mediaqueryresponse); replace this with a modern listener: prefer mql.addEventListener('change', mediaqueryresponse) but guard for older Safari by feature-detecting addEventListener and falling back to mql.addListener(mediaqueryresponse) if absent; also consider updating the media query string from 'screen and (max-device-width: 480px)' to 'screen and (max-width: 480px)' to use the Level 4 semantic (adjust only if compatible with supported browsers). Ensure you reference the mql variable and the mediaqueryresponse callback when implementing the conditional registration and fallback.
738-740: Modernise deprecated media query and event listener APIsBoth
max-device-width(deprecated in Media Queries Level 4; viewport-basedmax-widthis the modern replacement) andMediaQueryList.addListener()(deprecated;addEventListener('change', …)is the standard approach) are used here. Since the surrounding code is being touched, consider updating to modern equivalents.♻️ Proposed modernisation
- var mql = window.matchMedia('screen and (max-device-width: 480px)'); + var mql = window.matchMedia('screen and (max-width: 480px)'); mediaqueryresponse(mql); - mql.addListener(mediaqueryresponse); + mql.addEventListener('change', mediaqueryresponse);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/packages/rocketchat-livechat/assets/rocket-livechat.js` around lines 738 - 740, Update the media query to use viewport-based "max-width" instead of the deprecated "max-device-width" and replace the deprecated MediaQueryList.addListener call with the modern MediaQueryList.addEventListener('change', ...) pattern: create the MediaQueryList using window.matchMedia('screen and (max-width: 480px)'), call mediaqueryresponse(mql) as before, then attach the listener via mql.addEventListener('change', mediaqueryresponse) and include a fallback to mql.addListener(mediaqueryresponse) for older browsers that lack addEventListener; refer to the mql variable and the mediaqueryresponse callback to locate where to apply these changes.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/packages/rocketchat-livechat/assets/rocket-livechat.js
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/packages/rocketchat-livechat/assets/rocket-livechat.js
🧠 Learnings (1)
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
apps/meteor/packages/rocketchat-livechat/assets/rocket-livechat.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/meteor/packages/rocketchat-livechat/assets/rocket-livechat.js`:
- Around line 724-736: mediaqueryresponse currently flips smallScreen and widget
positioning but doesn't clear the mobile body scroll-lock when transitioning to
desktop while the widget is open; update mediaqueryresponse so that when
mql.matches is false and the widget is currently open (check the same
flag/condition openWidget uses), restore document.body.style.cssText from the
saved bodyStyle and restore scroll via scrollPosition before setting smallScreen
= false and applying desktop styles to chatWidget; reference mediaqueryresponse,
smallScreen, openWidget, closeWidget, bodyStyle, scrollPosition, and chatWidget
to locate and implement this restoration.
- Around line 724-736: mediaqueryresponse currently flips smallScreen on desktop
but doesn't undo the mobile body scroll-lock if the widget is open; update
mediaqueryresponse so that when switching from mobile to desktop (mql.matches
false) and the widget is open (check the widget's open state), restore
document.body.style.cssText = bodyStyle and restore scroll position (using
scrollPosition) before setting smallScreen = false; reference the
mediaqueryresponse closure variables widget, bodyStyle, scrollPosition,
smallScreen, and ensure this mirrors the cleanup done by closeWidget so the body
lock is not left active after a resize.
---
Nitpick comments:
In `@apps/meteor/packages/rocketchat-livechat/assets/rocket-livechat.js`:
- Around line 738-740: The code uses the deprecated MediaQueryList.addListener
via mql.addListener(mediaqueryresponse); replace this with a modern listener:
prefer mql.addEventListener('change', mediaqueryresponse) but guard for older
Safari by feature-detecting addEventListener and falling back to
mql.addListener(mediaqueryresponse) if absent; also consider updating the media
query string from 'screen and (max-device-width: 480px)' to 'screen and
(max-width: 480px)' to use the Level 4 semantic (adjust only if compatible with
supported browsers). Ensure you reference the mql variable and the
mediaqueryresponse callback when implementing the conditional registration and
fallback.
- Around line 738-740: Update the media query to use viewport-based "max-width"
instead of the deprecated "max-device-width" and replace the deprecated
MediaQueryList.addListener call with the modern
MediaQueryList.addEventListener('change', ...) pattern: create the
MediaQueryList using window.matchMedia('screen and (max-width: 480px)'), call
mediaqueryresponse(mql) as before, then attach the listener via
mql.addEventListener('change', mediaqueryresponse) and include a fallback to
mql.addListener(mediaqueryresponse) for older browsers that lack
addEventListener; refer to the mql variable and the mediaqueryresponse callback
to locate where to apply these changes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38764 +/- ##
===========================================
- Coverage 70.51% 70.47% -0.04%
===========================================
Files 3176 3176
Lines 111139 111139
Branches 20050 20016 -34
===========================================
- Hits 78367 78330 -37
- Misses 30721 30759 +38
+ Partials 2051 2050 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
1 participant
Proposed changes
Issue(s)
Closes #38763
How to test
Notes
This is a minimal one-line fix targeting the stale internal state.
Summary by CodeRabbit