[MM-58259] [MM-58260] Move the /call end command to host controls #747

cpoile · 2024-05-21T22:34:37Z

Summary

Move the /call end command to the host controls router
for [MM-58260]: Adjusted some of the checks in the toast and the channel link icon so that they properly disappear after /call end -- before they would stay visible after /call end, now they are gone. Let me know if there was a particular reason we were using profile count as a proxy for the call being active -- if so, then maybe should fix that instead.

Ticket Link

streamer45 · 2024-05-22T17:13:38Z

Let me know if there was a particular reason we were using profile count as a proxy for the call being active -- if so, then maybe should fix that instead.

I think there is a reason because I remember fixing related issues. I think this way you could still get the toast to show even when leaving a call (only you in the call). The call technically ends but we are not sending any event to clear the state.

cpoile · 2024-05-22T17:21:34Z

@streamer45 Ah, I see that now. So a couple thoughts, 1) I'll add an e2e test for that, cause that obviously shouldn't be happening when then call is over, 2) What do you think about sending a wsEventCallEnd event when we update the call's state EndAt? Since call is over, we should make sure the client knows it. Otherwise it feels like the participant counting is a hack.

streamer45 · 2024-05-22T17:31:57Z

@streamer45 Ah, I see that now. So a couple thoughts, 1) I'll add an e2e test for that, cause that obviously shouldn't be happening when then call is over, 2) What do you think about sending a wsEventCallEnd event when we update the call's state EndAt? Since call is over, we should make sure the client knows it. Otherwise it feels like the participant counting is a hack.

Yes, I think that makes sense. Let's just make sure this is tracked through the unique call ID so that we don't get into funny situations since a new call could start right after.

# Conflicts: # server/api.go

cpoile · 2024-05-27T14:40:52Z

webapp/src/index.tsx

@@ -327,7 +326,7 @@ export default class Plugin {

                // following the thread only on join. On call start
                // this is done in the call_start ws event handler.
-                if (profilesInCallInChannel(store.getState(), channelId).length > 0) {
+                if (channelHasCall(store.getState(), channelId)) {


The idea behind these refactorings are to move logic (like length tests) into the selectors, and make the selector's name declare what it does. This means more selectors, but it's clearer and the logic is isolated to one place.
Then, if we decide that counting participants is more correct, we can make that change in the selector.

cpoile · 2024-05-27T15:29:06Z

webapp/src/actions.ts

@@ -402,8 +402,14 @@ export const userLeft = (channelID: string, userID: string, sessionID: string) =
            },
        });

-        if (ringingEnabled(getState()) && !channelHasCall(getState(), channelID)) {
-            await dispatch(removeIncomingCallNotification(callID));
+        if (numSessionsInCallInChannel(getState(), channelID) === 0) {


Noting this because it's a big change. This is what we do in mobile and it seems to simplify things.

I implicitly thought we would handle this case server-side and send the unique call ID because this does not protect us against a race, as we still use the channel ID to figure out the locally stored call ID. It's not a huge deal, but it's something to note.

# Conflicts: # server/api.go

webapp/src/websocket_handlers.ts

streamer45

Thanks for taking the time to review this. The approach makes sense to me.

I am still seeing some weirdness but at this point I can't easily tell if it's a regression or not but still something we could look into as part of this PR.

If a user joins a call and then leaves I think we should show the toast once more. This doesn't seem to happen. What's weirder is that not even refreshing gets it back.

cpoile · 2024-05-27T22:09:56Z

@streamer45 We changed that behavior back in #445: dismissing the toast or joining (via toast or post button) dismisses the banner and it won't come back. If we want to change that behavior, then maybe we should open a new ticket?

streamer45 · 2024-05-27T22:28:29Z

@streamer45 We changed that behavior back in #445: dismissing the toast or joining (via toast or post button) dismisses the banner and it won't come back. If we want to change that behavior, then maybe we should open a new ticket?

Ah, forgot about that. Never mind then if it's working as designed.

streamer45

Looks good, nothing blocking.

streamer45 · 2024-05-27T22:35:46Z

webapp/src/actions.ts

@@ -402,8 +402,14 @@ export const userLeft = (channelID: string, userID: string, sessionID: string) =
            },
        });

-        if (ringingEnabled(getState()) && !channelHasCall(getState(), channelID)) {
-            await dispatch(removeIncomingCallNotification(callID));
+        if (numSessionsInCallInChannel(getState(), channelID) === 0) {


I implicitly thought we would handle this case server-side and send the unique call ID because this does not protect us against a race, as we still use the channel ID to figure out the locally stored call ID. It's not a huge deal, but it's something to note.

cpoile · 2024-05-28T14:13:39Z

@streamer45 Can you elaborate on what the race condition would be? I don't see how there could be one here...

streamer45 · 2024-05-28T14:52:01Z

@cpoile The only concern is a call in the same channel starting right after the previous one and incorrectly cleaning up the redux state for the latter one.

cpoile · 2024-05-28T15:07:41Z

@streamer45 I don't think that's possible since it's all synchronous now. (I removed an async that could let that happen, even if the chances of that would be incredibly small.)

streamer45 · 2024-05-28T15:11:15Z

Yeah, I haven't checked the whole path, including corner cases like WS reconnects, but if it worked fine on mobile until now, we should be good.

streamer45 · 2024-05-28T16:45:06Z

@cpoile Can we merge? So I can prepare a build for Community and start the soaking process.

move the /call end command to host controls

2323675

cpoile added the 2: Dev Review Requires review by a core committer label May 21, 2024

cpoile requested a review from streamer45 May 21, 2024 22:34

cpoile changed the title ~~[MM-58259] Move the /call end command to host controls~~ [MM-58259] [MM-58260] Move the /call end command to host controls May 21, 2024

goimports

d1ba0b6

streamer45 added this to the v0.28.0 / MM 9.10 milestone May 21, 2024

cpoile added 3 commits May 22, 2024 09:31

better data-testid; make selector more specific

e2486bb

clean up test

c36aa8d

test that second host can use /call end

4935f50

cpoile added 4 commits May 24, 2024 10:59

keep backward compatibility for mobile

8934ce0

send end_call event when sessions = 0; refactor selectors

021ebc4

Merge branch 'main' into MM-58259-end-call

9583d41

# Conflicts: # server/api.go

merge conflict

79cff72

cpoile commented May 27, 2024

View reviewed changes

simplify notification logic now we are always sending call_end action

e39e8b2

cpoile commented May 27, 2024

View reviewed changes

cpoile added 2 commits May 27, 2024 13:45

Merge branch 'main' into MM-58259-end-call

01088a7

# Conflicts: # server/api.go

merge conflict

b49ec0a

streamer45 reviewed May 27, 2024

View reviewed changes

webapp/src/websocket_handlers.ts Outdated Show resolved Hide resolved

remove mistaken async

b710a15

streamer45 reviewed May 27, 2024

View reviewed changes

streamer45 self-requested a review May 27, 2024 22:28

streamer45 approved these changes May 27, 2024

View reviewed changes

remove another unnecessary await

997e9ac

streamer45 added 3: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels May 28, 2024

cpoile merged commit b501c6b into main May 28, 2024
19 checks passed

cpoile deleted the MM-58259-end-call branch May 28, 2024 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-58259] [MM-58260] Move the /call end command to host controls #747

[MM-58259] [MM-58260] Move the /call end command to host controls #747

cpoile commented May 21, 2024 •

edited

streamer45 commented May 22, 2024

cpoile commented May 22, 2024

streamer45 commented May 22, 2024

cpoile May 27, 2024

cpoile May 27, 2024

streamer45 May 27, 2024

streamer45 left a comment

cpoile commented May 27, 2024

streamer45 commented May 27, 2024

streamer45 left a comment

streamer45 May 27, 2024

cpoile commented May 28, 2024

streamer45 commented May 28, 2024

cpoile commented May 28, 2024

streamer45 commented May 28, 2024

streamer45 commented May 28, 2024

[MM-58259] [MM-58260] Move the /call end command to host controls #747

[MM-58259] [MM-58260] Move the /call end command to host controls #747

Conversation

cpoile commented May 21, 2024 • edited

Summary

Ticket Link

streamer45 commented May 22, 2024

cpoile commented May 22, 2024

streamer45 commented May 22, 2024

cpoile May 27, 2024

Choose a reason for hiding this comment

cpoile May 27, 2024

Choose a reason for hiding this comment

streamer45 May 27, 2024

Choose a reason for hiding this comment

streamer45 left a comment

Choose a reason for hiding this comment

cpoile commented May 27, 2024

streamer45 commented May 27, 2024

streamer45 left a comment

Choose a reason for hiding this comment

streamer45 May 27, 2024

Choose a reason for hiding this comment

cpoile commented May 28, 2024

streamer45 commented May 28, 2024

cpoile commented May 28, 2024

streamer45 commented May 28, 2024

streamer45 commented May 28, 2024

cpoile commented May 21, 2024 •

edited