Skip to content

fix: Livechat conversation not closing due to cross-tab interference#35168

Merged
kodiakhq[bot] merged 5 commits intodevelopfrom
fix/close-chat-race-condition
Feb 17, 2025
Merged

fix: Livechat conversation not closing due to cross-tab interference#35168
kodiakhq[bot] merged 5 commits intodevelopfrom
fix/close-chat-race-condition

Conversation

@aleksandernsilva
Copy link
Contributor

@aleksandernsilva aleksandernsilva commented Feb 10, 2025

Proposed changes (including videos or screenshots)

This PR mitigates a race condition happening on livechat and being reported intermittently on FLAKY-652.

The problem occurs when there's more than one tab running Livechat instance. When Livechat.closeChat is called, all tabs receive the system message livechat-close and initiate the closeChat() procedure. After the first tab finished, all other tabs are blocked due to the room being already closed. This sometimes blocks the tab where the close chat request was originated, which is not the desired behavior.

This PR mitigates this problem by:

  1. Calling closeChat() only when the system message is received.
    This prevents the routine from being called twice. It unifies the logic, close chat requests initiated by either agent or visitor will follow the same routine. Finally, chat transcript request params are received via system message, the previous behavior could end up not displaying the transcript modal.
  2. Removing the room existence check from closeChat()
    Initially the check was added was to prevent closeChat from being called twice (onFinishedChat and system message), change 1. takes care of this aspect. Since the function only executes client side mutations, it executing more than once in the same instance is problematic, but by different instances is acceptable.

This is a palliative solution, since at this moment no form of mutex seems to allow for the cross-tab communication needed. This behavior will be revisited in the future for a more robust solution.

Issue(s)

Steps to test or reproduce

To reliably reproduce this issue it is necessary to have more than one tab running livechat (in sync) and intentionally delay the call to processMessage (room.js) in one of the instances.

const processMessage = async (message) => {
	if (message.t === 'livechat-close') {
		const params = new URLSearchParams(window.location.search);

		if (params.has('delay')) {
			await new Promise((res) => setTimeout(() => closeChat(message).then(res), 5000));
		} else {
			await closeChat(message);
		}
	}
[...]

This issue can also happen during normal use, but very sporadically.

Further comments

No new tests were added since our current tests already cover this specific scenario.

@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Feb 10, 2025

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Feb 10, 2025

🦋 Changeset detected

Latest commit: 7d45a97

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@rocket.chat/livechat Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codecov
Copy link

codecov bot commented Feb 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.44%. Comparing base (9f6a7b4) to head (7d45a97).
Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop   #35168   +/-   ##
========================================
  Coverage    59.44%   59.44%           
========================================
  Files         2827     2827           
  Lines        68162    68162           
  Branches     15123    15123           
========================================
  Hits         40516    40516           
  Misses       24990    24990           
  Partials      2656     2656           
Flag Coverage Δ
unit 75.69% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@aleksandernsilva aleksandernsilva marked this pull request as ready for review February 10, 2025 14:40
@aleksandernsilva aleksandernsilva requested review from a team as code owners February 10, 2025 14:40
@aleksandernsilva aleksandernsilva added this to the 7.4.0 milestone Feb 13, 2025
@aleksandernsilva aleksandernsilva added the stat: QA assured Means it has been tested and approved by a company insider label Feb 17, 2025
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Feb 17, 2025
@kodiakhq kodiakhq bot merged commit d4f6aa1 into develop Feb 17, 2025
47 of 48 checks passed
@kodiakhq kodiakhq bot deleted the fix/close-chat-race-condition branch February 17, 2025 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants