Connect to chat only after call is connected #4716
Conversation
rzdor
requested review from
JamesBurnside,
alkwa-msft,
dmceachernmsft,
carocao-msft,
edwardlee-msft,
PorterNan,
mgamis-msft,
prabhjot-msft,
AmyL219,
jimchou-dev,
vhuseinova-msft,
Leah-Xia-Microsoft,
jpeng-ms,
waynemo,
emlynmac,
JoshuaLai and
palatter
Calling bundle size is not changed.
|
CallWithChat bundle size is increased❗.
|
Chat bundle size is increased❗.
|
|
Failed to pass the Static HTML UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot. |
|
Failed to pass the UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot. |
|
Failed to pass the UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "update_snapshots" label to the PR for updating the snapshot. |
There was a problem hiding this comment.
Can we use the callWithChatAdapter to automatically join the chat thread once the call goes into the connected state? I see that its Contoso's call to the server we need to time calling the function appropriately.
I like your fix - perhaps we can remove the polling however?
We should probably have a feature in the backlog where we callback to Contoso to tell them when they should be triggering their join, but that can be a future feature
| if (threads.value.filter((thread: ChatThreadItem) => thread.id === this.chatThreadClient.threadId).length > 0) { | ||
| this.hasAcceccToThread = true; | ||
| } else { | ||
| setTimeout(check, 500); |
There was a problem hiding this comment.
Don't use polling. Instead just error if we try to run functions without access yet
There was a problem hiding this comment.
Does anything break if we don't poll?
There was a problem hiding this comment.
To remove the polling, we would have to have the CallWithChatComposite have some form of "retry" if the chat is opened before the user has connected.
Potentially this could be automatic (so in essence the same as polling).
Or we could have a retry button.
Or a combination of both.
E.g., a potential solution could look like:
- User joins call.
- User clicks chat button, sees error "You are not part of this chat thread" and a "Retry" button.
- Clicking retry shows a spinner, then shows the error again.
- At some point Contoso then adds the user to the chat thread in the backend.
- Ideally this would be done as early as possible to prevent the user ever seeing an error.
- Now if the user hits the retry button, or closes and reopens the chat it will load the chat successfully.
However, better would be if the CallWithChatComposite could subscribe to some heartbeat to automatically reload the chat when the user is added to the thread. This would need discussed with the Chat team.
|
|
||
| /* send add participant request to chat only after call is connected */ | ||
| const joinThreadWhenCallConnected = async (): Promise<void> => { | ||
| if (adapter.getState().call?.state === 'Connected' && locator && 'chatThreadId' in locator) { |
There was a problem hiding this comment.
Can we join in the "connecting" state? Just as an optimization that it's joined once the user lands in the call
| private async asyncTeeErrorToEventEmitter<T>(f: () => Promise<T>): Promise<T> { | ||
| try { | ||
| return await f(); | ||
| return await this.executeWithThreadAccess(() => { |
There was a problem hiding this comment.
This function is more for event emitter - it does not align with function name to add this function here, maybe we can do this in a separated function?
There was a problem hiding this comment.
For sure we can wrap the function for another time - but maybe extract what we have and make function for this
|
|
||
| private checkIsUserJoinedThread(): void { | ||
| const check = (): void => { | ||
| const threads = this.chatClient.listChatThreads({ maxPageSize: 100 }).byPage().next(); |
There was a problem hiding this comment.
I believe there is getThread or getChatThreadClient(id) function in the chatClient, let's not use iterator to make it slow
What
Fix bug when user joined chat before call and message about join appears first.
I've added call to join chat only after call is connected.
Updated ChatAdapter to wait when user joined thread for operations.
Why
How Tested
Process & policy checklist
Is this a breaking change?