Skip to content

Comments

[FIX] Fixing Jitsi call ended Issue.#21808

Merged
ggazzo merged 1 commit intoRocketChat:developfrom
yash-rajpal:fix-jitsi-issue
May 21, 2021
Merged

[FIX] Fixing Jitsi call ended Issue.#21808
ggazzo merged 1 commit intoRocketChat:developfrom
yash-rajpal:fix-jitsi-issue

Conversation

@yash-rajpal
Copy link
Member

  • I have read the Contributing Guide
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • I have added necessary documentation (if applicable)
  • Any dependent changes have been merged and published in downstream modules

Proposed changes (including videos or screenshots)

The new rewrite in react of contextual call component broke the Jitsi "click to join" messages. The issue being after 10 seconds of initiating the call, the message "click to join" always returned "Call Ended" even if the call was still going on.
This was due to the fact that after closing the contextual bar, the react component gets unmounted and we are not able to keep track of ongoing call and increase jitsi room timeout.

This PR solves this issue by using the setInterval methods on component will unmount. When the call component unmounts, we keep on checking the state of jitsi call and based on conditions increase the jitsi room timeout. After the call is ended all setInterval calls are closed.

This PR also removes the implementation of HEARTBEAT events of JitsiBridge. This is because this is no longer needed and all logic is being taken care of by the unmount function.

Issue(s)

#21055

Steps to test or reproduce

Further comments

@yash-rajpal
Copy link
Member Author

@tiagoevanp Please review it. :)

@emikolajczak
Copy link

Hi, is it possible to drop it on open.rocket.chat for tests?

@emikolajczak
Copy link

I don't know if this is deployed on https://open.rocket.chat , but still no working there. Not possible to join. Detail on movie below

bandicam.2021-04-28.08-59-58-358.mp4

@yash-rajpal
Copy link
Member Author

@emikolajczak This fix isn't released yet, as this pull request is not merged.

This fix will be available to test on open rocket chat server after this pr is merged and released.

@emikolajczak
Copy link

Understand. Do you know when can we expect release this?

@yash-rajpal
Copy link
Member Author

No, thats not in my hands, I can't say anything on this.

Its upon Rocket Chat team to review this pr and merge it if it looks good. After that we can expect it to be released in some release candidate, after that happens we will be able to test it on open rocket chat workspace.

@emikolajczak
Copy link

Thanks for explanation. I will wait.

@yash-rajpal
Copy link
Member Author

@tiagoevanp @ggazzo @sampaiodiego This seems like a useful pr and lots of community members are facing a problem due to this, please have a look. Thanks :)

Adding proof that this pr solves the problem.
Jitsi-call-ended-issue-fixed

Due to current issue, the button after 10 seconds tells that call ended, but by above GIF we can see that button is now functional and works as expected. I tested it thoroughly and everything seems to work as expected.
After ending the call button gives output call ended, which is the desired output.

@emikolajczak can you confirm if this is the required fix?

@HiveMindNet
Copy link

@tiagoevanp @ggazzo @sampaiodiego This seems like a useful pr and lots of community members are facing a problem due to this, please have a look. Thanks :)

Adding proof that this pr solves the problem.
Jitsi-call-ended-issue-fixed

Due to current issue, the button after 10 seconds tells that call ended, but by above GIF we can see that button is now functional and works as expected. I tested it thoroughly and everything seems to work as expected.
After ending the call button gives output call ended, which is the desired output.

@emikolajczak can you confirm if this is the required fix?

That's great work @yash-rajpal - it would be fantastic if the main guys could finish checks on this and include it in the main code @sampaiodiego

@sandrotanner
Copy link

whats the internal timeline to review this? @sampaiodiego

and in what time frame can we expect this to be merged? will there be a PATCH for all supported versions? or will it be in the next minor release? maybe in the version after the next?

This information could prove to be insightful in determining weather we want to wait or if it's better to patch it on our own for our server until it finds it's way into the official codebase.

@ankar84
Copy link

ankar84 commented May 6, 2021

Waiting that PR to be reviewed and then merged.
Thank you, @yash-rajpal

Copy link
Member

@ggazzo ggazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @yash-rajpal thanks for your pr, I'm afraid the HEARTBEAT cannot be removed.
I mean this 'protocol' was not meant for "you", but to inform other users that never opened the contextualbar if the call is still alive

could you review the fix and keep it? thanks

@yash-rajpal
Copy link
Member Author

yash-rajpal commented May 20, 2021

@ggazzo Can you please clarify more on your comment, on what has to be done? This PR fixes that issue only, to let other users know if call is still active or not, which is currently broken.

Do you want me to use JitsiBridge Class implementation, rather my own in the component?

@ggazzo ggazzo force-pushed the fix-jitsi-issue branch from 491f442 to 477b129 Compare May 21, 2021 03:33
@ggazzo
Copy link
Member

ggazzo commented May 21, 2021

please ignore my comment, and sorry i was running out of time for the release so i ended up changing the code a little bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants