From 744eeb53fe17caa3d1cf3e45876db042a82a5147 Mon Sep 17 00:00:00 2001 From: Robin Date: Tue, 15 Mar 2022 08:15:26 -0400 Subject: [PATCH] Don't assume that widget IDs are unique (#8052) * Don't assume that widget IDs are unique Signed-off-by: Robin Townsend * Don't remove live tiles that don't exist Signed-off-by: Robin Townsend * Add unit test for AppTile's live tile tracking Signed-off-by: Robin Townsend --- src/CallHandler.tsx | 2 +- .../views/context_menus/WidgetContextMenu.tsx | 2 +- src/components/views/elements/AppTile.tsx | 45 ++--- .../views/elements/PersistentApp.tsx | 25 +-- src/components/views/rooms/Stickerpicker.tsx | 4 +- src/components/views/voip/PipView.tsx | 38 +++-- src/stores/ActiveWidgetStore.ts | 58 +++---- src/stores/ModalWidgetStore.ts | 18 +- src/stores/WidgetStore.ts | 28 ++-- src/stores/widgets/StopGapWidget.ts | 17 +- src/stores/widgets/WidgetMessagingStore.ts | 35 ++-- src/utils/WidgetUtils.ts | 8 + .../views/elements/AppTile-test.tsx | 155 +++++++++++++++--- 13 files changed, 276 insertions(+), 159 deletions(-) diff --git a/src/CallHandler.tsx b/src/CallHandler.tsx index 07fc0bb1158..42d5aaba33b 100644 --- a/src/CallHandler.tsx +++ b/src/CallHandler.tsx @@ -1123,7 +1123,7 @@ export default class CallHandler extends EventEmitter { const jitsiWidgets = roomInfo.widgets.filter(w => WidgetType.JITSI.matches(w.type)); jitsiWidgets.forEach(w => { - const messaging = WidgetMessagingStore.instance.getMessagingForId(w.id); + const messaging = WidgetMessagingStore.instance.getMessagingForUid(WidgetUtils.getWidgetUid(w)); if (!messaging) return; // more "should never happen" words messaging.transport.send(ElementWidgetActions.HangupCall, {}); diff --git a/src/components/views/context_menus/WidgetContextMenu.tsx b/src/components/views/context_menus/WidgetContextMenu.tsx index f47871d4b59..1aeeccc724b 100644 --- a/src/components/views/context_menus/WidgetContextMenu.tsx +++ b/src/components/views/context_menus/WidgetContextMenu.tsx @@ -57,7 +57,7 @@ const WidgetContextMenu: React.FC = ({ const cli = useContext(MatrixClientContext); const { room, roomId } = useContext(RoomContext); - const widgetMessaging = WidgetMessagingStore.instance.getMessagingForId(app.id); + const widgetMessaging = WidgetMessagingStore.instance.getMessagingForUid(WidgetUtils.getWidgetUid(app)); const canModify = userWidget || WidgetUtils.canUserModifyWidgets(roomId); let streamAudioStreamButton; diff --git a/src/components/views/elements/AppTile.tsx b/src/components/views/elements/AppTile.tsx index b4fe7681644..0a33b23908e 100644 --- a/src/components/views/elements/AppTile.tsx +++ b/src/components/views/elements/AppTile.tsx @@ -118,24 +118,27 @@ export default class AppTile extends React.Component { showLayoutButtons: true, }; - // We track a count of all "live" `AppTile`s for a given widget ID. + // We track a count of all "live" `AppTile`s for a given widget UID. // For this purpose, an `AppTile` is considered live from the time it is // constructed until it is unmounted. This is used to aid logic around when // to tear down the widget iframe. See `componentWillUnmount` for details. - private static liveTilesById: { [key: string]: number } = {}; + private static liveTilesByUid = new Map(); - public static addLiveTile(widgetId: string): void { - const refs = this.liveTilesById[widgetId] || 0; - this.liveTilesById[widgetId] = refs + 1; + public static addLiveTile(widgetId: string, roomId: string): void { + const uid = WidgetUtils.calcWidgetUid(widgetId, roomId); + const refs = this.liveTilesByUid.get(uid) ?? 0; + this.liveTilesByUid.set(uid, refs + 1); } - public static removeLiveTile(widgetId: string): void { - const refs = this.liveTilesById[widgetId] || 0; - this.liveTilesById[widgetId] = refs - 1; + public static removeLiveTile(widgetId: string, roomId: string): void { + const uid = WidgetUtils.calcWidgetUid(widgetId, roomId); + const refs = this.liveTilesByUid.get(uid); + if (refs) this.liveTilesByUid.set(uid, refs - 1); } - public static isLive(widgetId: string): boolean { - const refs = this.liveTilesById[widgetId] || 0; + public static isLive(widgetId: string, roomId: string): boolean { + const uid = WidgetUtils.calcWidgetUid(widgetId, roomId); + const refs = this.liveTilesByUid.get(uid) ?? 0; return refs > 0; } @@ -150,10 +153,10 @@ export default class AppTile extends React.Component { constructor(props: IProps) { super(props); - AppTile.addLiveTile(this.props.app.id); + AppTile.addLiveTile(this.props.app.id, this.props.app.roomId); // The key used for PersistedElement - this.persistKey = getPersistKey(this.props.app.id); + this.persistKey = getPersistKey(WidgetUtils.getWidgetUid(this.props.app)); try { this.sgWidget = new StopGapWidget(this.props); this.setupSgListeners(); @@ -189,7 +192,9 @@ export default class AppTile extends React.Component { }; private onUserLeftRoom() { - const isActiveWidget = ActiveWidgetStore.instance.getWidgetPersistence(this.props.app.id); + const isActiveWidget = ActiveWidgetStore.instance.getWidgetPersistence( + this.props.app.id, this.props.app.roomId, + ); if (isActiveWidget) { // We just left the room that the active widget was from. if (this.props.room && RoomViewStore.getRoomId() !== this.props.room.roomId) { @@ -200,7 +205,7 @@ export default class AppTile extends React.Component { this.reload(); } else { // Otherwise just cancel its persistence. - ActiveWidgetStore.instance.destroyPersistentWidget(this.props.app.id); + ActiveWidgetStore.instance.destroyPersistentWidget(this.props.app.id, this.props.app.roomId); } } } @@ -241,7 +246,7 @@ export default class AppTile extends React.Component { if (this.state.hasPermissionToLoad && !hasPermissionToLoad) { // Force the widget to be non-persistent (able to be deleted/forgotten) - ActiveWidgetStore.instance.destroyPersistentWidget(this.props.app.id); + ActiveWidgetStore.instance.destroyPersistentWidget(this.props.app.id, this.props.app.roomId); PersistedElement.destroyElement(this.persistKey); this.sgWidget?.stopMessaging(); } @@ -291,14 +296,16 @@ export default class AppTile extends React.Component { // container is constructed before the old one unmounts. By counting the // mounted `AppTile`s for each widget, we know to only tear down the // widget iframe when the last the `AppTile` unmounts. - AppTile.removeLiveTile(this.props.app.id); + AppTile.removeLiveTile(this.props.app.id, this.props.app.roomId); // We also support a separate "persistence" mode where a single widget // can request to be "sticky" and follow you across rooms in a PIP // container. - const isActiveWidget = ActiveWidgetStore.instance.getWidgetPersistence(this.props.app.id); + const isActiveWidget = ActiveWidgetStore.instance.getWidgetPersistence( + this.props.app.id, this.props.app.roomId, + ); - if (!AppTile.isLive(this.props.app.id) && !isActiveWidget) { + if (!AppTile.isLive(this.props.app.id, this.props.app.roomId) && !isActiveWidget) { this.endWidgetActions(); } @@ -408,7 +415,7 @@ export default class AppTile extends React.Component { // Delete the widget from the persisted store for good measure. PersistedElement.destroyElement(this.persistKey); - ActiveWidgetStore.instance.destroyPersistentWidget(this.props.app.id); + ActiveWidgetStore.instance.destroyPersistentWidget(this.props.app.id, this.props.app.roomId); this.sgWidget?.stopMessaging({ forceDestroy: true }); } diff --git a/src/components/views/elements/PersistentApp.tsx b/src/components/views/elements/PersistentApp.tsx index 2961d7df01f..4883f50251b 100644 --- a/src/components/views/elements/PersistentApp.tsx +++ b/src/components/views/elements/PersistentApp.tsx @@ -16,8 +16,8 @@ limitations under the License. */ import React, { ContextType } from 'react'; +import { Room } from "matrix-js-sdk/src/models/room"; -import ActiveWidgetStore from '../../../stores/ActiveWidgetStore'; import WidgetUtils from '../../../utils/WidgetUtils'; import { replaceableComponent } from "../../../utils/replaceableComponent"; import AppTile from "./AppTile"; @@ -26,6 +26,7 @@ import MatrixClientContext from "../../../contexts/MatrixClientContext"; interface IProps { persistentWidgetId: string; + persistentRoomId: string; pointerEvents?: string; } @@ -33,32 +34,32 @@ interface IProps { export default class PersistentApp extends React.Component { public static contextType = MatrixClientContext; context: ContextType; + private room: Room; - private get app(): IApp { - const persistentWidgetInRoomId = ActiveWidgetStore.instance.getRoomId(this.props.persistentWidgetId); - const persistentWidgetInRoom = this.context.getRoom(persistentWidgetInRoomId); + constructor(props: IProps, context: ContextType) { + super(props, context); + this.room = context.getRoom(this.props.persistentRoomId); + } + private get app(): IApp { // get the widget data - const appEvent = WidgetUtils.getRoomWidgets(persistentWidgetInRoom).find((ev) => { - return ev.getStateKey() === ActiveWidgetStore.instance.getPersistentWidgetId(); - }); + const appEvent = WidgetUtils.getRoomWidgets(this.room).find(ev => + ev.getStateKey() === this.props.persistentWidgetId, + ); return WidgetUtils.makeAppConfig( appEvent.getStateKey(), appEvent.getContent(), appEvent.getSender(), - persistentWidgetInRoomId, appEvent.getId(), + this.room.roomId, appEvent.getId(), ); } public render(): JSX.Element { const app = this.app; if (app) { - const persistentWidgetInRoomId = ActiveWidgetStore.instance.getRoomId(this.props.persistentWidgetId); - const persistentWidgetInRoom = this.context.getRoom(persistentWidgetInRoomId); - return { private sendVisibilityToWidget(visible: boolean): void { if (!this.state.stickerpickerWidget) return; - const messaging = WidgetMessagingStore.instance.getMessagingForId(this.state.stickerpickerWidget.id); + const messaging = WidgetMessagingStore.instance.getMessagingForUid( + WidgetUtils.calcWidgetUid(this.state.stickerpickerWidget.id, null), + ); if (messaging && visible !== this.prevSentVisibility) { messaging.updateVisibility(visible).catch(err => { logger.error("Error updating widget visibility: ", err); diff --git a/src/components/views/voip/PipView.tsx b/src/components/views/voip/PipView.tsx index 1c3c9edc685..b3f3b0695df 100644 --- a/src/components/views/voip/PipView.tsx +++ b/src/components/views/voip/PipView.tsx @@ -60,6 +60,7 @@ interface IState { // widget candidate to be displayed in the pip view. persistentWidgetId: string; + persistentRoomId: string; showWidgetInPip: boolean; moving: boolean; @@ -122,6 +123,7 @@ export default class PipView extends React.Component { primaryCall: primaryCall, secondaryCall: secondaryCalls[0], persistentWidgetId: ActiveWidgetStore.instance.getPersistentWidgetId(), + persistentRoomId: ActiveWidgetStore.instance.getPersistentRoomId(), showWidgetInPip: false, }; } @@ -187,7 +189,10 @@ export default class PipView extends React.Component { }; private onActiveWidgetStoreUpdate = (): void => { - this.updateShowWidgetInPip(ActiveWidgetStore.instance.getPersistentWidgetId()); + this.updateShowWidgetInPip( + ActiveWidgetStore.instance.getPersistentWidgetId(), + ActiveWidgetStore.instance.getPersistentRoomId(), + ); }; private updateCalls = (): void => { @@ -213,30 +218,27 @@ export default class PipView extends React.Component { private onDoubleClick = (): void => { const callRoomId = this.state.primaryCall?.roomId; - const widgetRoomId = ActiveWidgetStore.instance.getRoomId(this.state.persistentWidgetId); - if (!!(callRoomId ?? widgetRoomId)) { + if (callRoomId ?? this.state.persistentRoomId) { dis.dispatch({ action: Action.ViewRoom, - room_id: callRoomId ?? widgetRoomId, + room_id: callRoomId ?? this.state.persistentRoomId, metricsTrigger: "WebFloatingCallWindow", }); } }; // Accepts a persistentWidgetId to be able to skip awaiting the setState for persistentWidgetId - public updateShowWidgetInPip(persistentWidgetId = this.state.persistentWidgetId) { + public updateShowWidgetInPip( + persistentWidgetId = this.state.persistentWidgetId, + persistentRoomId = this.state.persistentRoomId, + ) { let fromAnotherRoom = false; let notVisible = false; - if (persistentWidgetId) { - const persistentWidgetInRoomId = ActiveWidgetStore.instance.getRoomId(persistentWidgetId); - const persistentWidgetInRoom = MatrixClientPeg.get().getRoom(persistentWidgetInRoomId); - - // Sanity check the room - the widget may have been destroyed between render cycles, and - // thus no room is associated anymore. - if (persistentWidgetInRoom) { - notVisible = !AppTile.isLive(persistentWidgetId); - fromAnotherRoom = this.state.viewedRoomId !== persistentWidgetInRoomId; - } + // Sanity check the room - the widget may have been destroyed between render cycles, and + // thus no room is associated anymore. + if (persistentWidgetId && MatrixClientPeg.get().getRoom(persistentRoomId)) { + notVisible = !AppTile.isLive(persistentWidgetId, persistentRoomId); + fromAnotherRoom = this.state.viewedRoomId !== persistentRoomId; } // The widget should only be shown as a persistent app (in a floating @@ -245,7 +247,7 @@ export default class PipView extends React.Component { // containers of the room view. const showWidgetInPip = fromAnotherRoom || notVisible; - this.setState({ showWidgetInPip, persistentWidgetId }); + this.setState({ showWidgetInPip, persistentWidgetId, persistentRoomId }); } public render() { @@ -269,8 +271,7 @@ export default class PipView extends React.Component { mx_CallView_pip: pipMode, mx_CallView_large: !pipMode, }); - const roomId = ActiveWidgetStore.instance.getRoomId(this.state.persistentWidgetId); - const roomForWidget = MatrixClientPeg.get().getRoom(roomId); + const roomForWidget = MatrixClientPeg.get().getRoom(this.state.persistentRoomId); pipContent = ({ onStartMoving, _onResize }) =>
@@ -281,6 +282,7 @@ export default class PipView extends React.Component { />
; diff --git a/src/stores/ActiveWidgetStore.ts b/src/stores/ActiveWidgetStore.ts index 492332d3db2..28dea887df5 100644 --- a/src/stores/ActiveWidgetStore.ts +++ b/src/stores/ActiveWidgetStore.ts @@ -16,8 +16,10 @@ limitations under the License. import EventEmitter from 'events'; import { MatrixEvent, RoomStateEvent } from "matrix-js-sdk/src/matrix"; +import { RoomState } from "matrix-js-sdk/src/models/room-state"; import { MatrixClientPeg } from '../MatrixClientPeg'; +import WidgetUtils from "../utils/WidgetUtils"; import { WidgetMessagingStore } from "./widgets/WidgetMessagingStore"; export enum ActiveWidgetStoreEvent { @@ -33,8 +35,7 @@ export enum ActiveWidgetStoreEvent { export default class ActiveWidgetStore extends EventEmitter { private static internalInstance: ActiveWidgetStore; private persistentWidgetId: string; - // What room ID each widget is associated with (if it's a room widget) - private roomIdByWidgetId = new Map(); + private persistentRoomId: string; public static get instance(): ActiveWidgetStore { if (!ActiveWidgetStore.internalInstance) { @@ -48,64 +49,49 @@ export default class ActiveWidgetStore extends EventEmitter { } public stop(): void { - if (MatrixClientPeg.get()) { - MatrixClientPeg.get().removeListener(RoomStateEvent.Events, this.onRoomStateEvents); - } - this.roomIdByWidgetId.clear(); + MatrixClientPeg.get()?.removeListener(RoomStateEvent.Events, this.onRoomStateEvents); } - private onRoomStateEvents = (ev: MatrixEvent): void => { + private onRoomStateEvents = (ev: MatrixEvent, { roomId }: RoomState): void => { // XXX: This listens for state events in order to remove the active widget. // Everything else relies on views listening for events and calling setters // on this class which is terrible. This store should just listen for events // and keep itself up to date. // TODO: Enable support for m.widget event type (https://github.com/vector-im/element-web/issues/13111) - if (ev.getType() !== 'im.vector.modular.widgets') return; - - if (ev.getStateKey() === this.persistentWidgetId) { - this.destroyPersistentWidget(this.persistentWidgetId); + if (ev.getType() === "im.vector.modular.widgets") { + this.destroyPersistentWidget(ev.getStateKey(), roomId); } }; - public destroyPersistentWidget(id: string): void { - if (id !== this.persistentWidgetId) return; - const toDeleteId = this.persistentWidgetId; - - WidgetMessagingStore.instance.stopMessagingById(id); - - this.setWidgetPersistence(toDeleteId, false); - this.delRoomId(toDeleteId); + public destroyPersistentWidget(widgetId: string, roomId: string): void { + if (!this.getWidgetPersistence(widgetId, roomId)) return; + WidgetMessagingStore.instance.stopMessagingByUid(WidgetUtils.calcWidgetUid(widgetId, roomId)); + this.setWidgetPersistence(widgetId, roomId, false); } - public setWidgetPersistence(widgetId: string, val: boolean): void { - if (this.persistentWidgetId === widgetId && !val) { + public setWidgetPersistence(widgetId: string, roomId: string, val: boolean): void { + const isPersisted = this.getWidgetPersistence(widgetId, roomId); + + if (isPersisted && !val) { this.persistentWidgetId = null; - } else if (this.persistentWidgetId !== widgetId && val) { + this.persistentRoomId = null; + } else if (!isPersisted && val) { this.persistentWidgetId = widgetId; + this.persistentRoomId = roomId; } this.emit(ActiveWidgetStoreEvent.Update); } - public getWidgetPersistence(widgetId: string): boolean { - return this.persistentWidgetId === widgetId; + public getWidgetPersistence(widgetId: string, roomId: string): boolean { + return this.persistentWidgetId === widgetId && this.persistentRoomId === roomId; } public getPersistentWidgetId(): string { return this.persistentWidgetId; } - public getRoomId(widgetId: string): string { - return this.roomIdByWidgetId.get(widgetId); - } - - public setRoomId(widgetId: string, roomId: string): void { - this.roomIdByWidgetId.set(widgetId, roomId); - this.emit(ActiveWidgetStoreEvent.Update); - } - - public delRoomId(widgetId: string): void { - this.roomIdByWidgetId.delete(widgetId); - this.emit(ActiveWidgetStoreEvent.Update); + public getPersistentRoomId(): string { + return this.persistentRoomId; } } diff --git a/src/stores/ModalWidgetStore.ts b/src/stores/ModalWidgetStore.ts index 5bfb0a51a53..9c38b3f3a92 100644 --- a/src/stores/ModalWidgetStore.ts +++ b/src/stores/ModalWidgetStore.ts @@ -33,6 +33,7 @@ export class ModalWidgetStore extends AsyncStoreWithClient { private static internalInstance = new ModalWidgetStore(); private modalInstance: IHandle = null; private openSourceWidgetId: string = null; + private openSourceWidgetRoomId: string = null; private constructor() { super(defaultDispatcher, {}); @@ -57,31 +58,38 @@ export class ModalWidgetStore extends AsyncStoreWithClient { ) => { if (this.modalInstance) return; this.openSourceWidgetId = sourceWidget.id; + this.openSourceWidgetRoomId = widgetRoomId; this.modalInstance = Modal.createTrackedDialog('Modal Widget', '', ModalWidgetDialog, { widgetDefinition: { ...requestData }, widgetRoomId, sourceWidgetId: sourceWidget.id, onFinished: (success: boolean, data?: IModalWidgetReturnData) => { if (!success) { - this.closeModalWidget(sourceWidget, { "m.exited": true }); + this.closeModalWidget(sourceWidget, widgetRoomId, { "m.exited": true }); } else { - this.closeModalWidget(sourceWidget, data); + this.closeModalWidget(sourceWidget, widgetRoomId, data); } this.openSourceWidgetId = null; + this.openSourceWidgetRoomId = null; this.modalInstance = null; }, }, null, /* priority = */ false, /* static = */ true); }; - public closeModalWidget = (sourceWidget: Widget, data?: IModalWidgetReturnData) => { + public closeModalWidget = ( + sourceWidget: Widget, + widgetRoomId?: string, + data?: IModalWidgetReturnData, + ) => { if (!this.modalInstance) return; - if (this.openSourceWidgetId === sourceWidget.id) { + if (this.openSourceWidgetId === sourceWidget.id && this.openSourceWidgetRoomId === widgetRoomId) { this.openSourceWidgetId = null; + this.openSourceWidgetRoomId = null; this.modalInstance.close(); this.modalInstance = null; - const sourceMessaging = WidgetMessagingStore.instance.getMessaging(sourceWidget); + const sourceMessaging = WidgetMessagingStore.instance.getMessaging(sourceWidget, widgetRoomId); if (!sourceMessaging) { logger.error("No source widget messaging for modal widget"); return; diff --git a/src/stores/WidgetStore.ts b/src/stores/WidgetStore.ts index adec53d9caa..0d6ecb52526 100644 --- a/src/stores/WidgetStore.ts +++ b/src/stores/WidgetStore.ts @@ -29,7 +29,6 @@ import ActiveWidgetStore from "../stores/ActiveWidgetStore"; import WidgetUtils from "../utils/WidgetUtils"; import { WidgetType } from "../widgets/WidgetType"; import { UPDATE_EVENT } from "./AsyncStore"; -import { MatrixClientPeg } from "../MatrixClientPeg"; interface IState {} @@ -44,10 +43,6 @@ interface IRoomWidgets { widgets: IApp[]; } -function widgetUid(app: IApp): string { - return `${app.roomId ?? MatrixClientPeg.get().getUserId()}::${app.id}`; -} - // TODO consolidate WidgetEchoStore into this // TODO consolidate ActiveWidgetStore into this export default class WidgetStore extends AsyncStoreWithClient { @@ -119,13 +114,13 @@ export default class WidgetStore extends AsyncStoreWithClient { // otherwise we are out of sync with the rest of the app with stale widget events during removal Array.from(this.widgetMap.values()).forEach(app => { if (app.roomId !== room.roomId) return; // skip - wrong room - this.widgetMap.delete(widgetUid(app)); + this.widgetMap.delete(WidgetUtils.getWidgetUid(app)); }); let edited = false; this.generateApps(room).forEach(app => { // Sanity check for https://github.com/vector-im/element-web/issues/15705 - const existingApp = this.widgetMap.get(widgetUid(app)); + const existingApp = this.widgetMap.get(WidgetUtils.getWidgetUid(app)); if (existingApp) { logger.warn( `Possible widget ID conflict for ${app.id} - wants to store in room ${app.roomId} ` + @@ -133,7 +128,7 @@ export default class WidgetStore extends AsyncStoreWithClient { ); } - this.widgetMap.set(widgetUid(app), app); + this.widgetMap.set(WidgetUtils.getWidgetUid(app), app); roomInfo.widgets.push(app); edited = true; }); @@ -144,14 +139,13 @@ export default class WidgetStore extends AsyncStoreWithClient { // If a persistent widget is active, check to see if it's just been removed. // If it has, it needs to destroyed otherwise unmounting the node won't kill it const persistentWidgetId = ActiveWidgetStore.instance.getPersistentWidgetId(); - if (persistentWidgetId) { - if ( - ActiveWidgetStore.instance.getRoomId(persistentWidgetId) === room.roomId && - !roomInfo.widgets.some(w => w.id === persistentWidgetId) - ) { - logger.log(`Persistent widget ${persistentWidgetId} removed from room ${room.roomId}: destroying.`); - ActiveWidgetStore.instance.destroyPersistentWidget(persistentWidgetId); - } + if ( + persistentWidgetId && + ActiveWidgetStore.instance.getPersistentRoomId() === room.roomId && + !roomInfo.widgets.some(w => w.id === persistentWidgetId) + ) { + logger.log(`Persistent widget ${persistentWidgetId} removed from room ${room.roomId}: destroying.`); + ActiveWidgetStore.instance.destroyPersistentWidget(persistentWidgetId, room.roomId); } this.emit(room.roomId); @@ -196,7 +190,7 @@ export default class WidgetStore extends AsyncStoreWithClient { // A persistent conference widget indicates that we're participating const widgets = roomInfo.widgets.filter(w => WidgetType.JITSI.matches(w.type)); - return widgets.some(w => ActiveWidgetStore.instance.getWidgetPersistence(w.id)); + return widgets.some(w => ActiveWidgetStore.instance.getWidgetPersistence(w.id, room.roomId)); } } diff --git a/src/stores/widgets/StopGapWidget.ts b/src/stores/widgets/StopGapWidget.ts index 8dc2cbcfff4..0610927339c 100644 --- a/src/stores/widgets/StopGapWidget.ts +++ b/src/stores/widgets/StopGapWidget.ts @@ -264,11 +264,7 @@ export class StopGapWidget extends EventEmitter { this.messaging.on("ready", () => this.emit("ready")); this.messaging.on("capabilitiesNotified", () => this.emit("capabilitiesNotified")); this.messaging.on(`action:${WidgetApiFromWidgetAction.OpenModalWidget}`, this.onOpenModal); - WidgetMessagingStore.instance.storeMessaging(this.mockWidget, this.messaging); - - if (!this.appTileProps.userWidget && this.appTileProps.room) { - ActiveWidgetStore.instance.setRoomId(this.mockWidget.id, this.appTileProps.room.roomId); - } + WidgetMessagingStore.instance.storeMessaging(this.mockWidget, this.roomId, this.messaging); // Always attach a handler for ViewRoom, but permission check it internally this.messaging.on(`action:${ElementWidgetActions.ViewRoom}`, (ev: CustomEvent) => { @@ -318,7 +314,9 @@ export class StopGapWidget extends EventEmitter { this.messaging.on(`action:${WidgetApiFromWidgetAction.UpdateAlwaysOnScreen}`, (ev: CustomEvent) => { if (this.messaging.hasCapability(MatrixCapabilities.AlwaysOnScreen)) { - ActiveWidgetStore.instance.setWidgetPersistence(this.mockWidget.id, ev.detail.data.value); + ActiveWidgetStore.instance.setWidgetPersistence( + this.mockWidget.id, this.roomId, ev.detail.data.value, + ); ev.preventDefault(); this.messaging.transport.reply(ev.detail, {}); // ack } @@ -384,7 +382,7 @@ export class StopGapWidget extends EventEmitter { await (WidgetVariableCustomisations?.isReady?.() ?? Promise.resolve()); if (this.scalarToken) return; - const existingMessaging = WidgetMessagingStore.instance.getMessaging(this.mockWidget); + const existingMessaging = WidgetMessagingStore.instance.getMessaging(this.mockWidget, this.roomId); if (existingMessaging) this.messaging = existingMessaging; try { if (WidgetUtils.isScalarUrl(this.mockWidget.templateUrl)) { @@ -410,13 +408,12 @@ export class StopGapWidget extends EventEmitter { * @param opts */ public stopMessaging(opts = { forceDestroy: false }) { - if (!opts?.forceDestroy && ActiveWidgetStore.instance.getPersistentWidgetId() === this.mockWidget.id) { + if (!opts?.forceDestroy && ActiveWidgetStore.instance.getWidgetPersistence(this.mockWidget.id, this.roomId)) { logger.log("Skipping destroy - persistent widget"); return; } if (!this.started) return; - WidgetMessagingStore.instance.stopMessaging(this.mockWidget); - ActiveWidgetStore.instance.delRoomId(this.mockWidget.id); + WidgetMessagingStore.instance.stopMessaging(this.mockWidget, this.roomId); this.messaging = null; if (MatrixClientPeg.get()) { diff --git a/src/stores/widgets/WidgetMessagingStore.ts b/src/stores/widgets/WidgetMessagingStore.ts index 044832997eb..1766db27594 100644 --- a/src/stores/widgets/WidgetMessagingStore.ts +++ b/src/stores/widgets/WidgetMessagingStore.ts @@ -20,6 +20,7 @@ import { AsyncStoreWithClient } from "../AsyncStoreWithClient"; import defaultDispatcher from "../../dispatcher/dispatcher"; import { ActionPayload } from "../../dispatcher/payloads"; import { EnhancedMap } from "../../utils/maps"; +import WidgetUtils from "../../utils/WidgetUtils"; /** * Temporary holding store for widget messaging instances. This is eventually @@ -29,8 +30,7 @@ import { EnhancedMap } from "../../utils/maps"; export class WidgetMessagingStore extends AsyncStoreWithClient { private static internalInstance = new WidgetMessagingStore(); - // TODO: Fix uniqueness problem (widget IDs are not unique across the whole app) - private widgetMap = new EnhancedMap(); // + private widgetMap = new EnhancedMap(); // public constructor() { super(defaultDispatcher); @@ -49,35 +49,34 @@ export class WidgetMessagingStore extends AsyncStoreWithClient { this.widgetMap.clear(); } - public storeMessaging(widget: Widget, widgetApi: ClientWidgetApi) { - this.stopMessaging(widget); - this.widgetMap.set(widget.id, widgetApi); + public storeMessaging(widget: Widget, roomId: string, widgetApi: ClientWidgetApi) { + this.stopMessaging(widget, roomId); + this.widgetMap.set(WidgetUtils.calcWidgetUid(widget.id, roomId), widgetApi); } - public stopMessaging(widget: Widget) { - this.widgetMap.remove(widget.id)?.stop(); + public stopMessaging(widget: Widget, roomId: string) { + this.widgetMap.remove(WidgetUtils.calcWidgetUid(widget.id, roomId))?.stop(); } - public getMessaging(widget: Widget): ClientWidgetApi { - return this.widgetMap.get(widget.id); + public getMessaging(widget: Widget, roomId: string): ClientWidgetApi { + return this.widgetMap.get(WidgetUtils.calcWidgetUid(widget.id, roomId)); } /** - * Stops the widget messaging instance for a given widget ID. - * @param {string} widgetId The widget ID. - * @deprecated Widget IDs are not globally unique. + * Stops the widget messaging instance for a given widget UID. + * @param {string} widgetId The widget UID. */ - public stopMessagingById(widgetId: string) { - this.widgetMap.remove(widgetId)?.stop(); + public stopMessagingByUid(widgetUid: string) { + this.widgetMap.remove(widgetUid)?.stop(); } /** - * Gets the widget messaging class for a given widget ID. - * @param {string} widgetId The widget ID. + * Gets the widget messaging class for a given widget UID. + * @param {string} widgetId The widget UID. * @returns {ClientWidgetApi} The widget API, or a falsey value if not found. * @deprecated Widget IDs are not globally unique. */ - public getMessagingForId(widgetId: string): ClientWidgetApi { - return this.widgetMap.get(widgetId); + public getMessagingForUid(widgetUid: string): ClientWidgetApi { + return this.widgetMap.get(widgetUid); } } diff --git a/src/utils/WidgetUtils.ts b/src/utils/WidgetUtils.ts index c1d50cf0b77..02bd980f038 100644 --- a/src/utils/WidgetUtils.ts +++ b/src/utils/WidgetUtils.ts @@ -508,6 +508,14 @@ export default class WidgetUtils { return app?.data?.title?.trim() || ""; } + static getWidgetUid(app?: IApp): string { + return app ? WidgetUtils.calcWidgetUid(app.id, app.roomId) : ""; + } + + static calcWidgetUid(widgetId: string, roomId?: string): string { + return roomId ? `room_${roomId}_${widgetId}` : `user_${widgetId}`; + } + static editWidget(room: Room, app: IApp): void { // TODO: Open the right manager for the widget if (SettingsStore.getValue("feature_many_integration_managers")) { diff --git a/test/components/views/elements/AppTile-test.tsx b/test/components/views/elements/AppTile-test.tsx index 3d3779568d4..d10ca20bc28 100644 --- a/test/components/views/elements/AppTile-test.tsx +++ b/test/components/views/elements/AppTile-test.tsx @@ -44,7 +44,20 @@ describe("AppTile", () => { let r1; let r2; const resizeNotifier = new ResizeNotifier(); - let app: IApp; + let app1: IApp; + let app2: IApp; + + const waitForRps = (roomId: string) => new Promise(resolve => { + const update = () => { + if ( + RightPanelStore.instance.currentCardForRoom(roomId).phase !== + RightPanelPhases.Widget + ) return; + RightPanelStore.instance.off(UPDATE_EVENT, update); + resolve(); + }; + RightPanelStore.instance.on(UPDATE_EVENT, update); + }); beforeAll(async () => { stubClient(); @@ -66,18 +79,31 @@ describe("AppTile", () => { return [r1, r2]; }); - // Adjust various widget stores to add a mock app - app = { + // Adjust various widget stores to add mock apps + app1 = { id: "1", eventId: "1", roomId: "r1", type: MatrixWidgetType.Custom, url: "https://example.com", - name: "Example", + name: "Example 1", + creatorUserId: cli.getUserId(), + avatar_url: null, + }; + app2 = { + id: "1", + eventId: "2", + roomId: "r2", + type: MatrixWidgetType.Custom, + url: "https://example.com", + name: "Example 2", creatorUserId: cli.getUserId(), avatar_url: null, }; - jest.spyOn(WidgetStore.instance, "getApps").mockReturnValue([app]); + jest.spyOn(WidgetStore.instance, "getApps").mockImplementation(roomId => { + if (roomId === "r1") return [app1]; + if (roomId === "r2") return [app2]; + }); // Wake up various stores we rely on WidgetLayoutStore.instance.useUnitTestClient(cli); @@ -88,6 +114,26 @@ describe("AppTile", () => { await RightPanelStore.instance.onReady(); }); + it("tracks live tiles correctly", () => { + expect(AppTile.isLive("1", "r1")).toEqual(false); + + // Try removing the tile before it gets added + AppTile.removeLiveTile("1", "r1"); + expect(AppTile.isLive("1", "r1")).toEqual(false); + + AppTile.addLiveTile("1", "r1"); + expect(AppTile.isLive("1", "r1")).toEqual(true); + + AppTile.addLiveTile("1", "r1"); + expect(AppTile.isLive("1", "r1")).toEqual(true); + + AppTile.removeLiveTile("1", "r1"); + expect(AppTile.isLive("1", "r1")).toEqual(true); + + AppTile.removeLiveTile("1", "r1"); + expect(AppTile.isLive("1", "r1")).toEqual(false); + }); + it("destroys non-persisted right panel widget on room change", async () => { // Set up right panel state const realGetValue = SettingsStore.getValue; @@ -115,24 +161,14 @@ describe("AppTile", () => { /> ); // Wait for RPS room 1 updates to fire - const rpsUpdated = new Promise(resolve => { - const update = () => { - if ( - RightPanelStore.instance.currentCardForRoom("r1").phase !== - RightPanelPhases.Widget - ) return; - RightPanelStore.instance.off(UPDATE_EVENT, update); - resolve(); - }; - RightPanelStore.instance.on(UPDATE_EVENT, update); - }); + const rpsUpdated = waitForRps("r1"); dis.dispatch({ action: Action.ViewRoom, room_id: "r1", }); await rpsUpdated; - expect(AppTile.isLive("1")).toBe(true); + expect(AppTile.isLive("1", "r1")).toBe(true); // We want to verify that as we change to room 2, we should close the // right panel and destroy the widget. @@ -152,11 +188,88 @@ describe("AppTile", () => { ); expect(endWidgetActions.mock.calls.length).toBe(1); - expect(AppTile.isLive("1")).toBe(false); + expect(AppTile.isLive("1", "r1")).toBe(false); mockSettings.mockRestore(); }); + it("distinguishes widgets with the same ID in different rooms", async () => { + // Set up right panel state + const realGetValue = SettingsStore.getValue; + SettingsStore.getValue = (name, roomId) => { + if (name === "RightPanel.phases") { + if (roomId === "r1") { + return { + history: [{ + phase: RightPanelPhases.Widget, + state: { + widgetId: "1", + }, + }], + isOpen: true, + }; + } + return null; + } + return realGetValue(name, roomId); + }; + + // Run initial render with room 1, and also running lifecycle methods + const renderer = TestRenderer.create( + + ); + // Wait for RPS room 1 updates to fire + const rpsUpdated1 = waitForRps("r1"); + dis.dispatch({ + action: Action.ViewRoom, + room_id: "r1", + }); + await rpsUpdated1; + + expect(AppTile.isLive("1", "r1")).toBe(true); + expect(AppTile.isLive("1", "r2")).toBe(false); + + SettingsStore.getValue = (name, roomId) => { + if (name === "RightPanel.phases") { + if (roomId === "r2") { + return { + history: [{ + phase: RightPanelPhases.Widget, + state: { + widgetId: "1", + }, + }], + isOpen: true, + }; + } + return null; + } + return realGetValue(name, roomId); + }; + // Wait for RPS room 2 updates to fire + const rpsUpdated2 = waitForRps("r2"); + // Switch to room 2 + dis.dispatch({ + action: Action.ViewRoom, + room_id: "r2", + }); + renderer.update( + + ); + await rpsUpdated2; + + expect(AppTile.isLive("1", "r1")).toBe(false); + expect(AppTile.isLive("1", "r2")).toBe(true); + + SettingsStore.getValue = realGetValue; + }); + it("preserves non-persisted widget on container move", async () => { // Set up widget in top container const realGetValue = SettingsStore.getValue; @@ -187,7 +300,7 @@ describe("AppTile", () => { /> ); - expect(AppTile.isLive("1")).toBe(true); + expect(AppTile.isLive("1", "r1")).toBe(true); // We want to verify that as we move the widget to the center container, // the widget frame remains running. @@ -199,11 +312,11 @@ describe("AppTile", () => { // Stop mocking settings so that the widget move can take effect mockSettings.mockRestore(); TestRenderer.act(() => { - WidgetLayoutStore.instance.moveToContainer(r1, app, Container.Center); + WidgetLayoutStore.instance.moveToContainer(r1, app1, Container.Center); }); expect(endWidgetActions.mock.calls.length).toBe(0); - expect(AppTile.isLive("1")).toBe(true); + expect(AppTile.isLive("1", "r1")).toBe(true); }); afterAll(async () => {