fix(calling): fix the timer issue for call keepalive #4578
Conversation
rarajes2
force-pushed
the
fix-handleCallKeepalive-timer
branch
from
39c5696 to
7c64dea
Compare
adhmenon
left a comment
adhmenon
left a comment
There was a problem hiding this comment.
Have we tested with agent desktop?
I am a bit worried about that case as any change in timer logic might cause issues on their side since AD isn't exactly a browser app...
| } | ||
|
|
||
| this.sessionTimer = setInterval(async () => { | ||
| this.sessionTimer = setTimeout(async () => { |
There was a problem hiding this comment.
Let's discuss this once you are back. We should not remove setInterval itself in my opinion
There was a problem hiding this comment.
Sure, we can discuss this.
There was a problem hiding this comment.
Updated as discussed.
Tried testing on the AD but there seems to be some breaking change in the desktop login flow with the latest calling sdk. Please verify once and let me know if you are able to perform the desktop login with the latest calling SDK.
Not sure what do you mean by |
|
@adhmenon - Testing on the AD is not required for this. |
| log.log('Resetting session timer', loggerContext); | ||
|
|
||
| clearInterval(this.sessionTimer); | ||
| clearTimeout(this.sessionTimer); |
There was a problem hiding this comment.
This should be clearInterval I believe, also we may not need this block at all now because we are never calling Established handled in recurring manner anymore
There was a problem hiding this comment.
Right, this is not needed anymore. Removed.
| await Promise.resolve(); | ||
|
|
||
| expect(postStatusSpy).toHaveBeenCalledTimes(2); | ||
| expect(scheduleKeepaliveSpy).toHaveBeenCalled(); |
There was a problem hiding this comment.
How many times this has been called, add that please
|
Please add manual test results in the PR description. I would like to take a look |
3 participants
COMPLETES #< Ad-hoc >
This pull request addresses
Fixes the timer issue for the handleCallEstablished function.
This is continuation of the PR - #4557
by making the following changes
Clearing the interval in success case and rescheduling a new interval. Logic is bit altered to use
setTimeoutinstead ofsetIntervalnow.Change Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
Tested the following with the burp suite (to update the response header)
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.