fix(livechat): clean up widget teardown lifecycle#38745
fix(livechat): clean up widget teardown lifecycle#38745Shreyas2004wagh wants to merge 2 commits 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 |
|
WalkthroughAdds comprehensive widget lifecycle and teardown logic: stored listener callbacks, media-query listener handling, guarded navigation interval tracking, and teardown routines that remove listeners, stop intervals, restore DOM/scroll state, and prevent duplicate initialization on re-initialization. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Shreyas2004wagh
force-pushed
the
fix/livechat-widget-cleanup
branch
from
2e10515 to
14bc9fc
Compare
There was a problem hiding this comment.
Pull request overview
Cleans up the livechat widget teardown lifecycle to prevent listener/interval leaks across re-inits, across both the legacy injected asset and the TypeScript widget implementation.
Changes:
- Added explicit teardown helpers to stop navigation polling and detach
message+matchMedialisteners. - Prevented duplicate listener/interval attachment on repeated
init()calls by guarding and tearing down prior instances. - Updated
removeWidget()to run full teardown instead of only removing the DOM node.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/livechat/src/widget.ts | Adds tracked listener/interval management and a centralized teardownWidget() used on removeWidget() and re-init. |
| apps/meteor/packages/rocketchat-livechat/assets/rocket-livechat.js | Mirrors teardown behavior in the legacy widget asset, tracking listener fns/intervals so they can be detached on teardown. |
Comments suppressed due to low confidence (1)
packages/livechat/src/widget.ts:795
- On re-init,
trackNavigation()decides whether to send the initialpageVisited()messages based oncurrentPage.href/title, but those values aren't reset during teardown. After ateardownWidget()+init()on the same page/title, the new iframe session may never receive an initialpageVisitedevent until the URL/title changes. Consider resettingcurrentPage.href/currentPage.title(e.g., inteardownWidget()or before starting navigation tracking).
const trackNavigation = () => {
if (navigationIntervalId !== null) {
return;
}
navigationIntervalId = setInterval(() => {
if (document.location.href !== currentPage.href) {
pageVisited('url');
currentPage.href = document.location.href;
}
if (document.title !== currentPage.title) {
pageVisited('title');
currentPage.title = document.title;
}
}, 800);
};
const init = (url: string) => {
const trimmedUrl = url.trim();
if (!trimmedUrl) {
return;
}
if (widget) {
teardownWidget();
}
config.url = trimmedUrl;
createWidget(trimmedUrl);
attachMessageListener();
trackNavigation();
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/packages/rocketchat-livechat/assets/rocket-livechat.jspackages/livechat/src/widget.ts
🧰 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:
packages/livechat/src/widget.tsapps/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
🧬 Code graph analysis (2)
packages/livechat/src/widget.ts (2)
apps/meteor/client/lib/cachedStores/CachedStore.ts (1)
eventName(75-80)apps/meteor/server/lib/callbacks.ts (1)
callbacks(218-226)
apps/meteor/packages/rocketchat-livechat/assets/rocket-livechat.js (2)
packages/livechat/src/widget.ts (1)
ready(493-496)packages/core-services/src/index.ts (1)
api(55-55)
⏰ 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). (1)
- GitHub Check: 📦 Build Packages
🔇 Additional comments (12)
apps/meteor/packages/rocketchat-livechat/assets/rocket-livechat.js (6)
485-488: LGTM!Clean initialization of module-scoped tracking variables with
nulldefaults, consistent with the guard checks in the new helper functions.
520-551: LGTM!The three detach/stop helpers follow a consistent guard → cleanup → reset pattern, and
detachMediaQueryListenercorrectly handles both modern and legacyMediaQueryListAPIs.
553-574: LGTM!The teardown sequence correctly orchestrates cleanup: stops intervals, detaches all listeners, conditionally restores body styles/scroll when the widget was open on a small screen, removes the DOM node, and resets internal state.
734-748: LGTM!The guard at the top of
trackNavigationprevents duplicate intervals from accumulating across re-initializations.
756-758: Clean re-initialization guard.Calling
teardownWidget()before re-creating the widget ininitensures no duplicate listeners or orphaned DOM nodes survive a re-initialization cycle.
783-813: LGTM!Storing the message listener and media-query listener as named references enables reliable teardown. The cross-browser fallback for
addListener/addEventListenermirrors the detach logic correctly.packages/livechat/src/widget.ts (6)
83-87: LGTM!Well-typed module-scoped state variables for tracking callback handlers, navigation interval, message listener attachment, and media query resources.
89-116: Good fix for callback identity tracking.Storing handlers in a
Map<string, Set<...>>and using those exact references inclearAllCallbacksresolves the handler identity mismatch that previously prevented proper unregistration.
230-241: LGTM!Media query listener setup is properly stored in module scope for later teardown, with cross-browser compatibility for both
addEventListenerand the deprecatedaddListener.
704-720: LGTM!Using a boolean flag is appropriate here since
onNewMessageis a stable module-level reference. The guards prevent duplicate attachment and safe no-op detachment.
762-778: LGTM!Guard prevents duplicate interval creation, consistent with the JS counterpart.
786-788: LGTM!Pre-initialization teardown ensures a clean slate for re-initialization, preventing listener and interval accumulation in SPA scenarios.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/livechat/src/widget.ts`:
- Around line 746-760: The teardownWidget function removes the mobile
full-screen class but doesn't restore the saved scroll position; update
teardownWidget to set document.documentElement.scrollTop (or window.scrollTo)
back to the previously saved scrollPosition and restore any body inline styles
you saved when opening the widget (e.g., document.body.style.position/overflow)
before clearing widget/iframe and resetting ready/hookQueue so mobile scroll is
returned to its prior state; locate teardownWidget and the saved scrollPosition
variable to add these restores right after the class removal and before nulling
widget/iframe.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38745 +/- ##
===========================================
- Coverage 70.51% 70.48% -0.03%
===========================================
Files 3176 3176
Lines 111139 111139
Branches 20050 20033 -17
===========================================
- Hits 78367 78338 -29
- Misses 30721 30758 +37
+ Partials 2051 2043 -8
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/meteor/packages/rocketchat-livechat/assets/rocket-livechat.js (2)
504-510: LegacyregisterCallbackdoesn't track handler references for unregistration.Unlike
widget.tswhich uses acallbackHandlersmap to store handler references for proper cleanup inclearAllCallbacks, this legacy JS version registers callbacks on the emitter without storing references. Since the legacy JS doesn't exposeclearAllCallbacksin its public API and the scope of this file is more limited, this is likely acceptable — but worth noting for parity if callback cleanup is ever needed here.🤖 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 504 - 510, The registerCallback function registers event handlers but doesn't retain references for later unregistration; update registerCallback to mirror widget.ts by creating/using a callbackHandlers map keyed by eventName and storing the returned handler reference when calling callbacks.on(eventName, fn), so clearAllCallbacks (or future unregistration) can iterate callbackHandlers and call callbacks.off/unregister using those stored references; touch the registerCallback function, validCallbacks usage, the callbacks.on call, and ensure any clearAllCallbacks logic operates against the new callbackHandlers map.
553-578:teardownWidgetcorrectly restores body styles and scroll on mobile.The guarded restoration of
bodyStyleandscrollPosition(Lines 559–563) only fires when the widget is open on a small screen, which is the correct behavior. DOM removal and state reset follow in the right order.Minor nit: the
if (currentPage)guard on Line 574 is always truthy sincecurrentPageis a module-scoped object literal (Line 734), but it's harmless.🤖 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 553 - 578, TeardownWidget contains an unnecessary truthiness guard around currentPage (the module-scoped object literal currentPage is always truthy), so remove the redundant if (currentPage) check in teardownWidget and directly null out currentPage.href and currentPage.title; update references to currentPage inside teardownWidget (function teardownWidget) to simply set currentPage.href = null and currentPage.title = null to make intent clear and avoid a misleading conditional.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/packages/rocketchat-livechat/assets/rocket-livechat.jspackages/livechat/src/widget.ts
🧰 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:
packages/livechat/src/widget.tsapps/meteor/packages/rocketchat-livechat/assets/rocket-livechat.js
🧬 Code graph analysis (1)
apps/meteor/packages/rocketchat-livechat/assets/rocket-livechat.js (2)
packages/livechat/src/widget.ts (1)
ready(493-496)packages/core-services/src/index.ts (1)
api(55-55)
⏰ 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
🔇 Additional comments (8)
packages/livechat/src/widget.ts (5)
83-87: Good addition of lifecycle tracking state.The module-scoped variables for tracking navigation interval, message listener attachment, media query list/listener provide the foundation for proper cleanup. Implementation looks correct.
704-720: Guard against duplicate message listeners is correct.The
messageListenerAttachedboolean flag prevents stacking listeners across re-initializations. ThedetachMessageListener/attachMessageListenerpair is symmetric and clean.
746-767: Scroll restoration on mobile teardown now addressed — verifydocument.body.classListcleanup is sufficient.Lines 751–754 restore scroll position when the widget is open on a small screen, addressing the previously flagged concern. The
typeof scrollPosition === 'number'guard correctly handles the case wherescrollPositionwas never set.Note that unlike the legacy JS version (which restores
document.body.style.cssText), this version only removes the CSS class (Line 755). This is consistent with how the TS widget manages mobile styles (viarc-livechat-mobile-full-screenclass rather than inline styles), so this is correct.
769-785: Navigation interval guard prevents duplicate intervals.The
navigationIntervalId !== nullcheck at the top oftrackNavigationcorrectly prevents multiple concurrent intervals from being created on re-initialization.
787-801: Re-initialization tears down existing widget first — good.Calling
teardownWidget()before creating a new widget prevents duplicate DOM elements, listeners, and intervals. This directly addresses the SPA re-mount concern from the issue.apps/meteor/packages/rocketchat-livechat/assets/rocket-livechat.js (3)
485-488: Lifecycle state variables correctly mirror the TypeScript implementation.The four new state variables (
navigationInterval,messageListener,mediaQueryList,mediaQueryListener) enable proper teardown by retaining references to active listeners/timers.
787-795: StoringmessageListenerreference enables proper cleanup — good fix.Previously the anonymous function couldn't be removed by
removeEventListener. Assigning it to the module-scopedmessageListenervariable and passing that to bothaddEventListenerandremoveEventListener(indetachMessageListener) is the correct approach.
738-753: Navigation guard and init teardown are consistent with the TS implementation.
trackNavigationnow correctly prevents duplicate intervals, andinittears down any existing widget before re-initializing. Both patterns matchwidget.ts.Also applies to: 760-762
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/livechat/src/widget.ts`:
- Around line 89-99: teardownWidget currently doesn't clear callback handlers
registered via registerCallback, so callbacks in callbackHandlers persist across
widget teardown/re-init; decide desired behavior and implement accordingly: if
you want full cleanup call clearAllCallbacks() inside teardownWidget (or invoke
callbackHandlers.clear() there) so registered handlers are removed, otherwise
add a comment in teardownWidget explaining that retaining callbacks is
intentional to preserve user handlers across re-initialization and document that
behavior; reference registerCallback, teardownWidget, clearAllCallbacks, and
callbackHandlers when making the change.
---
Nitpick comments:
In `@apps/meteor/packages/rocketchat-livechat/assets/rocket-livechat.js`:
- Around line 504-510: The registerCallback function registers event handlers
but doesn't retain references for later unregistration; update registerCallback
to mirror widget.ts by creating/using a callbackHandlers map keyed by eventName
and storing the returned handler reference when calling callbacks.on(eventName,
fn), so clearAllCallbacks (or future unregistration) can iterate
callbackHandlers and call callbacks.off/unregister using those stored
references; touch the registerCallback function, validCallbacks usage, the
callbacks.on call, and ensure any clearAllCallbacks logic operates against the
new callbackHandlers map.
- Around line 553-578: TeardownWidget contains an unnecessary truthiness guard
around currentPage (the module-scoped object literal currentPage is always
truthy), so remove the redundant if (currentPage) check in teardownWidget and
directly null out currentPage.href and currentPage.title; update references to
currentPage inside teardownWidget (function teardownWidget) to simply set
currentPage.href = null and currentPage.title = null to make intent clear and
avoid a misleading conditional.
| function registerCallback(eventName: string, fn: (...args: unknown[]) => unknown) { | ||
| if (VALID_CALLBACKS.indexOf(eventName) === -1) { | ||
| return false; | ||
| } | ||
|
|
||
| const handlers = callbackHandlers.get(eventName) ?? new Set(); | ||
| handlers.add(fn); | ||
| callbackHandlers.set(eventName, handlers); | ||
|
|
||
| return callbacks.on(eventName, fn); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'clearAllCallbacks|teardownWidget|function init' packages/livechat/src/widget.ts -A 5 -B 2Repository: RocketChat/Rocket.Chat
Length of output: 1134
🏁 Script executed:
sed -n '746,800p' packages/livechat/src/widget.tsRepository: RocketChat/Rocket.Chat
Length of output: 1236
Confirm whether cleanup of registered callbacks on widget teardown is intentional.
teardownWidget() does not invoke clearAllCallbacks(). If the goal is full cleanup when the widget is removed or re-initialized, callbacks registered through registerCallback will persist. Either document this as intentional behavior (to preserve user callbacks across re-initialization) or call clearAllCallbacks() from teardownWidget().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/livechat/src/widget.ts` around lines 89 - 99, teardownWidget
currently doesn't clear callback handlers registered via registerCallback, so
callbacks in callbackHandlers persist across widget teardown/re-init; decide
desired behavior and implement accordingly: if you want full cleanup call
clearAllCallbacks() inside teardownWidget (or invoke callbackHandlers.clear()
there) so registered handlers are removed, otherwise add a comment in
teardownWidget explaining that retaining callbacks is intentional to preserve
user handlers across re-initialization and document that behavior; reference
registerCallback, teardownWidget, clearAllCallbacks, and callbackHandlers when
making the change.
1 participant
Summary
Validation
Notes
Fix #38746
Summary by CodeRabbit