From 5c26d580d81a3319008c8fe9b274406e80735456 Mon Sep 17 00:00:00 2001 From: Timo <16718859+toger5@users.noreply.github.com> Date: Mon, 17 Jun 2024 13:00:41 +0200 Subject: [PATCH] Fix widgets not being cleaned up correctly. (#12616) * Fix widgets not being cleaned up correctly. Widgets could persist forever because they were still sticky when we end the messaging. Ending the messaging emits an event to which we connect ui changes that move the widget out of the screen. It does not end up in a pip view however. So we need to make sure the widget is not persistend anymore when we call `stopMessagingByUid` so that any dom changes that remove the AppTile happen when the widget is not persistend anymore and let it destroy. This PR also makes the role for `MatrixRTCSessionManager` more strict. We do ONLY use it in `Call.ts` and `CallStore` so that we dont end up in reaces where we updated the ui based on the session manager but not in sync with the call and callstore changes. Rename activeCalls to connectedCalls. Active call can also be understood as a call where there are active participants but the user itself is not connected. Especially with the `hasActiveCallSession` field of the useRoomCall hook which is tracking active (not necassarly connected sessions) * rest of the renaming * fix test to adapt to reduced session manager event usage. --- src/components/structures/MatrixChat.tsx | 2 +- src/components/structures/RoomView.tsx | 6 ++-- src/components/views/voip/CallView.tsx | 2 +- src/hooks/room/useRoomCall.ts | 8 ++--- src/models/Call.ts | 2 +- src/stores/ActiveWidgetStore.ts | 6 +++- src/stores/CallStore.ts | 30 +++++++++---------- src/stores/room-list/algorithms/Algorithm.ts | 10 +++---- src/stores/widgets/StopGapWidget.ts | 3 +- src/toasts/IncomingCallToast.tsx | 30 ++++++++----------- .../components/structures/MatrixChat-test.tsx | 2 +- .../views/rooms/RoomHeader-test.tsx | 4 +-- test/toasts/IncomingCallToast-test.tsx | 14 ++------- 13 files changed, 54 insertions(+), 65 deletions(-) diff --git a/src/components/structures/MatrixChat.tsx b/src/components/structures/MatrixChat.tsx index adee652751..92e13786cf 100644 --- a/src/components/structures/MatrixChat.tsx +++ b/src/components/structures/MatrixChat.tsx @@ -647,7 +647,7 @@ export default class MatrixChat extends React.PureComponent { case "logout": LegacyCallHandler.instance.hangupAllCalls(); Promise.all([ - ...[...CallStore.instance.activeCalls].map((call) => call.disconnect()), + ...[...CallStore.instance.connectedCalls].map((call) => call.disconnect()), cleanUpBroadcasts(this.stores), ]).finally(() => Lifecycle.logout(this.stores.oidcClientStore)); break; diff --git a/src/components/structures/RoomView.tsx b/src/components/structures/RoomView.tsx index 3c51bf53fd..ceeb638dd9 100644 --- a/src/components/structures/RoomView.tsx +++ b/src/components/structures/RoomView.tsx @@ -494,7 +494,7 @@ export class RoomView extends React.Component { WidgetEchoStore.on(UPDATE_EVENT, this.onWidgetEchoStoreUpdate); context.widgetStore.on(UPDATE_EVENT, this.onWidgetStoreUpdate); - CallStore.instance.on(CallStoreEvent.ActiveCalls, this.onActiveCalls); + CallStore.instance.on(CallStoreEvent.ConnectedCalls, this.onConnectedCalls); this.props.resizeNotifier.on("isResizing", this.onIsResizing); @@ -815,7 +815,7 @@ export class RoomView extends React.Component { } }; - private onActiveCalls = (): void => { + private onConnectedCalls = (): void => { if (this.state.roomId === undefined) return; const activeCall = CallStore.instance.getActiveCall(this.state.roomId); if (activeCall === null) { @@ -1056,7 +1056,7 @@ export class RoomView extends React.Component { ); } - CallStore.instance.off(CallStoreEvent.ActiveCalls, this.onActiveCalls); + CallStore.instance.off(CallStoreEvent.ConnectedCalls, this.onConnectedCalls); this.context.legacyCallHandler.off(LegacyCallHandlerEvent.CallState, this.onCallState); // cancel any pending calls to the throttled updated diff --git a/src/components/views/voip/CallView.tsx b/src/components/views/voip/CallView.tsx index ce7ed253a7..772d50b082 100644 --- a/src/components/views/voip/CallView.tsx +++ b/src/components/views/voip/CallView.tsx @@ -60,7 +60,7 @@ const JoinCallView: FC = ({ room, resizing, call, skipLobby, const disconnectAllOtherCalls: () => Promise = useCallback(async () => { // The stickyPromise has to resolve before the widget actually becomes sticky. // We only let the widget become sticky after disconnecting all other active calls. - const calls = [...CallStore.instance.activeCalls].filter( + const calls = [...CallStore.instance.connectedCalls].filter( (call) => SdkContextClass.instance.roomViewStore.getRoomId() !== call.roomId, ); await Promise.all(calls.map(async (call) => await call.disconnect())); diff --git a/src/hooks/room/useRoomCall.ts b/src/hooks/room/useRoomCall.ts index 376a7dbb5f..57145235fa 100644 --- a/src/hooks/room/useRoomCall.ts +++ b/src/hooks/room/useRoomCall.ts @@ -176,13 +176,13 @@ export const useRoomCall = ( // We only want to prompt to pin the widget if it's not element call based. const isECWidget = WidgetType.CALL.matches(widget?.type ?? ""); const promptPinWidget = !isECWidget && canPinWidget && !widgetPinned; - const activeCalls = useEventEmitterState(CallStore.instance, CallStoreEvent.ActiveCalls, () => - Array.from(CallStore.instance.activeCalls), + const connectedCalls = useEventEmitterState(CallStore.instance, CallStoreEvent.ConnectedCalls, () => + Array.from(CallStore.instance.connectedCalls), ); const { canInviteGuests } = useGuestAccessInformation(room); const state = useMemo((): State => { - if (activeCalls.find((call) => call.roomId != room.roomId)) { + if (connectedCalls.find((call) => call.roomId != room.roomId)) { return State.Ongoing; } if (hasGroupCall && (hasJitsiWidget || hasManagedHybridWidget)) { @@ -200,7 +200,7 @@ export const useRoomCall = ( } return State.NoCall; }, [ - activeCalls, + connectedCalls, canInviteGuests, hasGroupCall, hasJitsiWidget, diff --git a/src/models/Call.ts b/src/models/Call.ts index c7d53284cc..7c42719563 100644 --- a/src/models/Call.ts +++ b/src/models/Call.ts @@ -963,7 +963,7 @@ export class ElementCall extends Call { private onRTCSessionEnded = (roomId: string, session: MatrixRTCSession): void => { // Don't destroy the call on hangup for video call rooms. - if (roomId == this.roomId && !this.room.isCallRoom()) { + if (roomId === this.roomId && !this.room.isCallRoom()) { this.destroy(); } }; diff --git a/src/stores/ActiveWidgetStore.ts b/src/stores/ActiveWidgetStore.ts index ef67892102..214f2aa673 100644 --- a/src/stores/ActiveWidgetStore.ts +++ b/src/stores/ActiveWidgetStore.ts @@ -70,8 +70,12 @@ export default class ActiveWidgetStore extends EventEmitter { public destroyPersistentWidget(widgetId: string, roomId: string | null): void { if (!this.getWidgetPersistence(widgetId, roomId)) return; - WidgetMessagingStore.instance.stopMessagingByUid(WidgetUtils.calcWidgetUid(widgetId, roomId ?? undefined)); + // We first need to set the widget persistence to false this.setWidgetPersistence(widgetId, roomId, false); + // Then we can stop the messaging. Stopping the messaging emits - we might move the widget out of sight. + // If we would do this before setting the persistence to false, it would stay in the DOM (hidden) because + // its still persistent. We need to avoid this. + WidgetMessagingStore.instance.stopMessagingByUid(WidgetUtils.calcWidgetUid(widgetId, roomId ?? undefined)); } public setWidgetPersistence(widgetId: string, roomId: string | null, val: boolean): void { diff --git a/src/stores/CallStore.ts b/src/stores/CallStore.ts index c32a542133..0187f3d5aa 100644 --- a/src/stores/CallStore.ts +++ b/src/stores/CallStore.ts @@ -34,7 +34,7 @@ export enum CallStoreEvent { // Signals a change in the call associated with a given room Call = "call", // Signals a change in the active calls - ActiveCalls = "active_calls", + ConnectedCalls = "connected_calls", } export class CallStore extends AsyncStoreWithClient<{}> { @@ -66,8 +66,7 @@ export class CallStore extends AsyncStoreWithClient<{}> { } this.matrixClient.on(GroupCallEventHandlerEvent.Incoming, this.onGroupCall); this.matrixClient.on(GroupCallEventHandlerEvent.Outgoing, this.onGroupCall); - this.matrixClient.matrixRTC.on(MatrixRTCSessionManagerEvents.SessionStarted, this.onRTCSession); - this.matrixClient.matrixRTC.on(MatrixRTCSessionManagerEvents.SessionEnded, this.onRTCSession); + this.matrixClient.matrixRTC.on(MatrixRTCSessionManagerEvents.SessionStarted, this.onRTCSessionStart); WidgetStore.instance.on(UPDATE_EVENT, this.onWidgets); // If the room ID of a previously connected call is still in settings at @@ -95,28 +94,27 @@ export class CallStore extends AsyncStoreWithClient<{}> { } this.callListeners.clear(); this.calls.clear(); - this._activeCalls.clear(); + this._connectedCalls.clear(); if (this.matrixClient) { this.matrixClient.off(GroupCallEventHandlerEvent.Incoming, this.onGroupCall); this.matrixClient.off(GroupCallEventHandlerEvent.Outgoing, this.onGroupCall); this.matrixClient.off(GroupCallEventHandlerEvent.Ended, this.onGroupCall); - this.matrixClient.matrixRTC.off(MatrixRTCSessionManagerEvents.SessionStarted, this.onRTCSession); - this.matrixClient.matrixRTC.off(MatrixRTCSessionManagerEvents.SessionEnded, this.onRTCSession); + this.matrixClient.matrixRTC.off(MatrixRTCSessionManagerEvents.SessionStarted, this.onRTCSessionStart); } WidgetStore.instance.off(UPDATE_EVENT, this.onWidgets); } - private _activeCalls: Set = new Set(); + private _connectedCalls: Set = new Set(); /** * The calls to which the user is currently connected. */ - public get activeCalls(): Set { - return this._activeCalls; + public get connectedCalls(): Set { + return this._connectedCalls; } - private set activeCalls(value: Set) { - this._activeCalls = value; - this.emit(CallStoreEvent.ActiveCalls, value); + private set connectedCalls(value: Set) { + this._connectedCalls = value; + this.emit(CallStoreEvent.ConnectedCalls, value); // The room IDs are persisted to settings so we can detect unclean disconnects SettingsStore.setValue( @@ -137,9 +135,9 @@ export class CallStore extends AsyncStoreWithClient<{}> { if (call) { const onConnectionState = (state: ConnectionState): void => { if (state === ConnectionState.Connected) { - this.activeCalls = new Set([...this.activeCalls, call]); + this.connectedCalls = new Set([...this.connectedCalls, call]); } else if (state === ConnectionState.Disconnected) { - this.activeCalls = new Set([...this.activeCalls].filter((c) => c !== call)); + this.connectedCalls = new Set([...this.connectedCalls].filter((c) => c !== call)); } }; const onDestroy = (): void => { @@ -181,7 +179,7 @@ export class CallStore extends AsyncStoreWithClient<{}> { */ public getActiveCall(roomId: string): Call | null { const call = this.getCall(roomId); - return call !== null && this.activeCalls.has(call) ? call : null; + return call !== null && this.connectedCalls.has(call) ? call : null; } private onWidgets = (roomId: string | null): void => { @@ -200,7 +198,7 @@ export class CallStore extends AsyncStoreWithClient<{}> { }; private onGroupCall = (groupCall: GroupCall): void => this.updateRoom(groupCall.room); - private onRTCSession = (roomId: string, session: MatrixRTCSession): void => { + private onRTCSessionStart = (roomId: string, session: MatrixRTCSession): void => { this.updateRoom(session.room); }; } diff --git a/src/stores/room-list/algorithms/Algorithm.ts b/src/stores/room-list/algorithms/Algorithm.ts index 267b9bd742..6e28eb1a4e 100644 --- a/src/stores/room-list/algorithms/Algorithm.ts +++ b/src/stores/room-list/algorithms/Algorithm.ts @@ -85,11 +85,11 @@ export class Algorithm extends EventEmitter { public updatesInhibited = false; public start(): void { - CallStore.instance.on(CallStoreEvent.ActiveCalls, this.onActiveCalls); + CallStore.instance.on(CallStoreEvent.ConnectedCalls, this.onConnectedCalls); } public stop(): void { - CallStore.instance.off(CallStoreEvent.ActiveCalls, this.onActiveCalls); + CallStore.instance.off(CallStoreEvent.ConnectedCalls, this.onConnectedCalls); } public get stickyRoom(): Room | null { @@ -302,7 +302,7 @@ export class Algorithm extends EventEmitter { return this._stickyRoom; } - private onActiveCalls = (): void => { + private onConnectedCalls = (): void => { // In case we're unsticking a room, sort it back into natural order this.recalculateStickyRoom(); @@ -396,12 +396,12 @@ export class Algorithm extends EventEmitter { return; } - if (CallStore.instance.activeCalls.size) { + if (CallStore.instance.connectedCalls.size) { // We operate on the sticky rooms map if (!this._cachedStickyRooms) this.initCachedStickyRooms(); const rooms = this._cachedStickyRooms![updatedTag]; - const activeRoomIds = new Set([...CallStore.instance.activeCalls].map((call) => call.roomId)); + const activeRoomIds = new Set([...CallStore.instance.connectedCalls].map((call) => call.roomId)); const activeRooms: Room[] = []; const inactiveRooms: Room[] = []; diff --git a/src/stores/widgets/StopGapWidget.ts b/src/stores/widgets/StopGapWidget.ts index 36ade5d594..b08190cd92 100644 --- a/src/stores/widgets/StopGapWidget.ts +++ b/src/stores/widgets/StopGapWidget.ts @@ -345,7 +345,6 @@ export class StopGapWidget extends EventEmitter { async (ev: CustomEvent) => { if (this.messaging?.hasCapability(MatrixCapabilities.AlwaysOnScreen)) { ev.preventDefault(); - this.messaging.transport.reply(ev.detail, {}); // ack if (ev.detail.data.value) { // If the widget wants to become sticky we wait for the stickyPromise to resolve if (this.stickyPromise) await this.stickyPromise(); @@ -356,6 +355,8 @@ export class StopGapWidget extends EventEmitter { this.roomId ?? null, ev.detail.data.value, ); + // Send the ack after the widget actually has become sticky. + this.messaging.transport.reply(ev.detail, {}); } }, ); diff --git a/src/toasts/IncomingCallToast.tsx b/src/toasts/IncomingCallToast.tsx index dfdf25efe6..e0b7944495 100644 --- a/src/toasts/IncomingCallToast.tsx +++ b/src/toasts/IncomingCallToast.tsx @@ -16,10 +16,6 @@ limitations under the License. import React, { useCallback, useEffect, useMemo, useState } from "react"; import { MatrixEvent } from "matrix-js-sdk/src/matrix"; -// eslint-disable-next-line no-restricted-imports -import { MatrixRTCSessionManagerEvents } from "matrix-js-sdk/src/matrixrtc/MatrixRTCSessionManager"; -// eslint-disable-next-line no-restricted-imports -import { MatrixRTCSession } from "matrix-js-sdk/src/matrixrtc/MatrixRTCSession"; import { Button, Tooltip } from "@vector-im/compound-web"; import { Icon as VideoCallIcon } from "@vector-im/compound-design-tokens/icons/video-call-solid.svg"; @@ -41,7 +37,7 @@ import { useDispatcher } from "../hooks/useDispatcher"; import { ActionPayload } from "../dispatcher/payloads"; import { Call } from "../models/Call"; import { AudioID } from "../LegacyCallHandler"; -import { useEventEmitter, useTypedEventEmitter } from "../hooks/useEventEmitter"; +import { useEventEmitter } from "../hooks/useEventEmitter"; import { CallStore, CallStoreEvent } from "../stores/CallStore"; export const getIncomingCallToastKey = (callId: string, roomId: string): string => `call_${callId}_${roomId}`; @@ -83,11 +79,11 @@ export function IncomingCallToast({ notifyEvent }: Props): JSX.Element { const room = MatrixClientPeg.safeGet().getRoom(roomId) ?? undefined; const call = useCall(roomId); const audio = useMemo(() => document.getElementById(AudioID.Ring) as HTMLMediaElement, []); - const [activeCalls, setActiveCalls] = useState(Array.from(CallStore.instance.activeCalls)); - useEventEmitter(CallStore.instance, CallStoreEvent.ActiveCalls, () => { - setActiveCalls(Array.from(CallStore.instance.activeCalls)); + const [connectedCalls, setConnectedCalls] = useState(Array.from(CallStore.instance.connectedCalls)); + useEventEmitter(CallStore.instance, CallStoreEvent.ConnectedCalls, () => { + setConnectedCalls(Array.from(CallStore.instance.connectedCalls)); }); - const otherCallIsOngoing = activeCalls.find((call) => call.roomId !== roomId); + const otherCallIsOngoing = connectedCalls.find((call) => call.roomId !== roomId); // Start ringing if not already. useEffect(() => { const isRingToast = (notifyEvent.getContent() as unknown as { notify_type: string })["notify_type"] == "ring"; @@ -105,13 +101,15 @@ export function IncomingCallToast({ notifyEvent }: Props): JSX.Element { }, [audio, notifyEvent, roomId]); // Dismiss if session got ended remotely. - const onSessionEnded = useCallback( - (endedSessionRoomId: string, session: MatrixRTCSession): void => { - if (roomId == endedSessionRoomId && session.callId == notifyEvent.getContent().call_id) { + const onCall = useCallback( + (call: Call, callRoomId: string): void => { + const roomId = notifyEvent.getRoomId(); + if (!roomId && roomId !== callRoomId) return; + if (call === null || call.participants.size === 0) { dismissToast(); } }, - [dismissToast, notifyEvent, roomId], + [dismissToast, notifyEvent], ); // Dismiss on timeout. @@ -160,11 +158,7 @@ export function IncomingCallToast({ notifyEvent }: Props): JSX.Element { [dismissToast], ); - useTypedEventEmitter( - MatrixClientPeg.safeGet().matrixRTC, - MatrixRTCSessionManagerEvents.SessionEnded, - onSessionEnded, - ); + useEventEmitter(CallStore.instance, CallStoreEvent.Call, onCall); return ( <> diff --git a/test/components/structures/MatrixChat-test.tsx b/test/components/structures/MatrixChat-test.tsx index 8ce80eed03..d2d4178d1f 100644 --- a/test/components/structures/MatrixChat-test.tsx +++ b/test/components/structures/MatrixChat-test.tsx @@ -769,7 +769,7 @@ describe("", () => { jest.spyOn(PosthogAnalytics.instance, "logout").mockImplementation(() => {}); jest.spyOn(EventIndexPeg, "deleteEventIndex").mockImplementation(async () => {}); - jest.spyOn(CallStore.instance, "activeCalls", "get").mockReturnValue(new Set([call1, call2])); + jest.spyOn(CallStore.instance, "connectedCalls", "get").mockReturnValue(new Set([call1, call2])); mockPlatformPeg({ destroyPickleKey: jest.fn(), diff --git a/test/components/views/rooms/RoomHeader-test.tsx b/test/components/views/rooms/RoomHeader-test.tsx index 21a5cccf2b..a3cb89a7fb 100644 --- a/test/components/views/rooms/RoomHeader-test.tsx +++ b/test/components/views/rooms/RoomHeader-test.tsx @@ -492,7 +492,7 @@ describe("RoomHeader", () => { it("buttons are disabled if there is an ongoing call", async () => { mockRoomMembers(room, 3); - jest.spyOn(CallStore.prototype, "activeCalls", "get").mockReturnValue( + jest.spyOn(CallStore.prototype, "connectedCalls", "get").mockReturnValue( new Set([{ roomId: "some_other_room" } as Call]), ); const { container } = render(, getWrapper()); @@ -514,7 +514,7 @@ describe("RoomHeader", () => { it("join button is disabled if there is an other ongoing call", async () => { mockRoomMembers(room, 3); jest.spyOn(UseCall, "useParticipantCount").mockReturnValue(3); - jest.spyOn(CallStore.prototype, "activeCalls", "get").mockReturnValue( + jest.spyOn(CallStore.prototype, "connectedCalls", "get").mockReturnValue( new Set([{ roomId: "some_other_room" } as Call]), ); const { container } = render(, getWrapper()); diff --git a/test/toasts/IncomingCallToast-test.tsx b/test/toasts/IncomingCallToast-test.tsx index d24ddae249..1745e2f036 100644 --- a/test/toasts/IncomingCallToast-test.tsx +++ b/test/toasts/IncomingCallToast-test.tsx @@ -20,10 +20,6 @@ import { mocked, Mocked } from "jest-mock"; import { Room, RoomStateEvent, MatrixEvent, MatrixEventEvent, MatrixClient } from "matrix-js-sdk/src/matrix"; import { ClientWidgetApi, Widget } from "matrix-widget-api"; // eslint-disable-next-line no-restricted-imports -import { MatrixRTCSessionManagerEvents } from "matrix-js-sdk/src/matrixrtc/MatrixRTCSessionManager"; -// eslint-disable-next-line no-restricted-imports -import { MatrixRTCSession } from "matrix-js-sdk/src/matrixrtc/MatrixRTCSession"; -// eslint-disable-next-line no-restricted-imports import { ICallNotifyContent } from "matrix-js-sdk/src/matrixrtc/types"; import type { RoomMember } from "matrix-js-sdk/src/matrix"; @@ -82,6 +78,7 @@ describe("IncomingCallEvent", () => { client.getRoom.mockImplementation((roomId) => (roomId === room.roomId ? room : null)); client.getRooms.mockReturnValue([room]); client.reEmitter.reEmit(room, [RoomStateEvent.Events]); + MockedCall.create(room, "1"); await Promise.all( [CallStore.instance, WidgetMessagingStore.instance].map((store) => @@ -89,7 +86,6 @@ describe("IncomingCallEvent", () => { ), ); - MockedCall.create(room, "1"); const maybeCall = CallStore.instance.getCall(room.roomId); if (!(maybeCall instanceof MockedCall)) throw new Error("Failed to create call"); call = maybeCall; @@ -179,7 +175,7 @@ describe("IncomingCallEvent", () => { defaultDispatcher.unregister(dispatcherRef); }); - it("skips lobby when using shift key click", async () => { + it("Dismiss toast if user starts call and skips lobby when using shift key click", async () => { renderToast(); const dispatcherSpy = jest.fn(); @@ -250,11 +246,7 @@ describe("IncomingCallEvent", () => { it("closes toast when the matrixRTC session has ended", async () => { renderToast(); - - client.matrixRTC.emit(MatrixRTCSessionManagerEvents.SessionEnded, room.roomId, { - callId: notifyContent.call_id, - room: room, - } as unknown as MatrixRTCSession); + call.destroy(); await waitFor(() => expect(toastStore.dismissToast).toHaveBeenCalledWith(