From bd1de8d45b953cc9f87c310121600921b2111ce2 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 1 Dec 2020 13:05:48 -0700 Subject: [PATCH 1/2] Require a room ID for WidgetStore's pinned widget contracts This should alleviate https://github.com/vector-im/element-web/issues/15705 from happening, though the cause is still unknown. Requiring a room ID is safe for this because only room widgets can be pinned, and widget IDs are not globally unique which means from a logical standpoint the contract still makes sense here. --- .../views/context_menus/WidgetContextMenu.tsx | 6 ++--- .../views/messages/MJitsiWidgetEvent.tsx | 2 +- .../views/right_panel/RoomSummaryCard.tsx | 13 ++++----- .../views/right_panel/WidgetCard.tsx | 2 +- src/stores/WidgetStore.ts | 27 +++++++++---------- 5 files changed, 24 insertions(+), 26 deletions(-) diff --git a/src/components/views/context_menus/WidgetContextMenu.tsx b/src/components/views/context_menus/WidgetContextMenu.tsx index 7656e70341..8026942038 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 = ({ let unpinButton; if (showUnpin) { const onUnpinClick = () => { - WidgetStore.instance.unpinWidget(app.id); + WidgetStore.instance.unpinWidget(room.roomId, app.id); onFinished(); }; @@ -143,7 +143,7 @@ const WidgetContextMenu: React.FC = ({ let moveLeftButton; if (showUnpin && widgetIndex > 0) { const onClick = () => { - WidgetStore.instance.movePinnedWidget(app.id, -1); + WidgetStore.instance.movePinnedWidget(roomId, app.id, -1); onFinished(); }; @@ -153,7 +153,7 @@ const WidgetContextMenu: React.FC = ({ let moveRightButton; if (showUnpin && widgetIndex < pinnedWidgets.length - 1) { const onClick = () => { - WidgetStore.instance.movePinnedWidget(app.id, 1); + WidgetStore.instance.movePinnedWidget(roomId, app.id, 1); onFinished(); }; diff --git a/src/components/views/messages/MJitsiWidgetEvent.tsx b/src/components/views/messages/MJitsiWidgetEvent.tsx index 82aa32d3b7..b87efd472a 100644 --- a/src/components/views/messages/MJitsiWidgetEvent.tsx +++ b/src/components/views/messages/MJitsiWidgetEvent.tsx @@ -35,7 +35,7 @@ export default class MJitsiWidgetEvent extends React.PureComponent { const senderName = this.props.mxEvent.sender?.name || this.props.mxEvent.getSender(); let joinCopy = _t('Join the conference at the top of this room'); - if (!WidgetStore.instance.isPinned(this.props.mxEvent.getStateKey())) { + if (!WidgetStore.instance.isPinned(this.props.mxEvent.getRoomId(), this.props.mxEvent.getStateKey())) { joinCopy = _t('Join the conference from the room information card on the right'); } diff --git a/src/components/views/right_panel/RoomSummaryCard.tsx b/src/components/views/right_panel/RoomSummaryCard.tsx index 621e85e1d4..4ce4b75f9b 100644 --- a/src/components/views/right_panel/RoomSummaryCard.tsx +++ b/src/components/views/right_panel/RoomSummaryCard.tsx @@ -83,9 +83,10 @@ export const useWidgets = (room: Room) => { interface IAppRowProps { app: IApp; + room: Room; } -const AppRow: React.FC = ({ app }) => { +const AppRow: React.FC = ({ app, room }) => { const name = WidgetUtils.getWidgetName(app); const dataTitle = WidgetUtils.getWidgetDataTitle(app); const subtitle = dataTitle && " - " + dataTitle; @@ -100,10 +101,10 @@ const AppRow: React.FC = ({ app }) => { }); }; - const isPinned = WidgetStore.instance.isPinned(app.id); + const isPinned = WidgetStore.instance.isPinned(room.roomId, app.id); const togglePin = isPinned - ? () => { WidgetStore.instance.unpinWidget(app.id); } - : () => { WidgetStore.instance.pinWidget(app.id); }; + ? () => { WidgetStore.instance.unpinWidget(room.roomId, app.id); } + : () => { WidgetStore.instance.pinWidget(room.roomId, app.id); }; const [menuDisplayed, handle, openMenu, closeMenu] = useContextMenu(); let contextMenu; @@ -118,7 +119,7 @@ const AppRow: React.FC = ({ app }) => { />; } - const cannotPin = !isPinned && !WidgetStore.instance.canPin(app.id); + const cannotPin = !isPinned && !WidgetStore.instance.canPin(room.roomId, app.id); let pinTitle: string; if (cannotPin) { @@ -183,7 +184,7 @@ const AppsSection: React.FC = ({ room }) => { }; return - { apps.map(app => ) } + { apps.map(app => ) } { apps.length > 0 ? _t("Edit widgets, bridges & bots") : _t("Add widgets, bridges & bots") } diff --git a/src/components/views/right_panel/WidgetCard.tsx b/src/components/views/right_panel/WidgetCard.tsx index c1753e90e3..593bd0dde7 100644 --- a/src/components/views/right_panel/WidgetCard.tsx +++ b/src/components/views/right_panel/WidgetCard.tsx @@ -42,7 +42,7 @@ const WidgetCard: React.FC = ({ room, widgetId, onClose }) => { const apps = useWidgets(room); const app = apps.find(a => a.id === widgetId); - const isPinned = app && WidgetStore.instance.isPinned(app.id); + const isPinned = app && WidgetStore.instance.isPinned(room.roomId, app.id); const [menuDisplayed, handle, openMenu, closeMenu] = useContextMenu(); diff --git a/src/stores/WidgetStore.ts b/src/stores/WidgetStore.ts index a8040f57de..42a230d53a 100644 --- a/src/stores/WidgetStore.ts +++ b/src/stores/WidgetStore.ts @@ -163,44 +163,42 @@ export default class WidgetStore extends AsyncStoreWithClient { this.emit(UPDATE_EVENT); }; - public isPinned(widgetId: string) { - const roomId = this.getRoomId(widgetId); + public isPinned(roomId: string, widgetId: string) { return !!this.getPinnedApps(roomId).find(w => w.id === widgetId); } - public canPin(widgetId: string) { - const roomId = this.getRoomId(widgetId); + // dev note: we don't need the widgetId on this function, but the contract makes more sense + // when we require it. + public canPin(roomId: string, widgetId: string) { return this.getPinnedApps(roomId).length < MAX_PINNED; } - public pinWidget(widgetId: string) { - const roomId = this.getRoomId(widgetId); + public pinWidget(roomId: string, widgetId: string) { const roomInfo = this.getRoom(roomId); if (!roomInfo) return; // When pinning, first confirm all the widgets (Jitsi) which were autopinned so that the order is correct const autoPinned = this.getPinnedApps(roomId).filter(app => !roomInfo.pinned[app.id]); autoPinned.forEach(app => { - this.setPinned(app.id, true); + this.setPinned(roomId, app.id, true); }); - this.setPinned(widgetId, true); + this.setPinned(roomId, widgetId, true); // Show the apps drawer upon the user pinning a widget if (RoomViewStore.getRoomId() === this.getRoomId(widgetId)) { defaultDispatcher.dispatch({ action: "appsDrawer", show: true, - }) + }); } } - public unpinWidget(widgetId: string) { - this.setPinned(widgetId, false); + public unpinWidget(roomId: string, widgetId: string) { + this.setPinned(roomId, widgetId, false); } - private setPinned(widgetId: string, value: boolean) { - const roomId = this.getRoomId(widgetId); + private setPinned(roomId: string, widgetId: string, value: boolean) { const roomInfo = this.getRoom(roomId); if (!roomInfo) return; if (roomInfo.pinned[widgetId] === false && value) { @@ -221,9 +219,8 @@ export default class WidgetStore extends AsyncStoreWithClient { this.emit(UPDATE_EVENT); } - public movePinnedWidget(widgetId: string, delta: 1 | -1) { + public movePinnedWidget(roomId: string, widgetId: string, delta: 1 | -1) { // TODO simplify this by changing the storage medium of pinned to an array once the Jitsi default-on goes away - const roomId = this.getRoomId(widgetId); const roomInfo = this.getRoom(roomId); if (!roomInfo || roomInfo.pinned[widgetId] === false) return; From 5df693205143f6ae0e5de84abe68b0ae67eefc3e Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 1 Dec 2020 13:19:51 -0700 Subject: [PATCH 2/2] Add various amounts of sanity checking for widget pinning This should also help https://github.com/vector-im/element-web/issues/15705 by either implicitly fixing the problem, causing chaos as described in the issue, or by forcing a crash to identify the problem more easily. --- src/stores/WidgetStore.ts | 42 +++++++++++++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/src/stores/WidgetStore.ts b/src/stores/WidgetStore.ts index 42a230d53a..8e08fc016c 100644 --- a/src/stores/WidgetStore.ts +++ b/src/stores/WidgetStore.ts @@ -39,9 +39,11 @@ export interface IApp extends IWidget { avatar_url: string; // MSC2765 https://github.com/matrix-org/matrix-doc/pull/2765 } +type PinnedWidgets = Record; + interface IRoomWidgets { widgets: IApp[]; - pinned: Record; + pinned: PinnedWidgets; } export const MAX_PINNED = 3; @@ -51,8 +53,9 @@ export const MAX_PINNED = 3; export default class WidgetStore extends AsyncStoreWithClient { private static internalInstance = new WidgetStore(); - private widgetMap = new Map(); - private roomMap = new Map(); + // TODO: Come up with a unique key for widgets as their IDs are not globally unique, but can exist anywhere + private widgetMap = new Map(); // Key is widget ID + private roomMap = new Map(); // Key is room ID private constructor() { super(defaultDispatcher, {}); @@ -132,6 +135,15 @@ export default class WidgetStore extends AsyncStoreWithClient { }); this.generateApps(room).forEach(app => { + // Sanity check for https://github.com/vector-im/element-web/issues/15705 + const existingApp = this.widgetMap.get(app.id); + if (existingApp) { + console.warn( + `Possible widget ID conflict for ${app.id} - wants to store in room ${app.roomId} ` + + `but is currently stored as ${existingApp.roomId} - letting the want win`, + ); + } + this.widgetMap.set(app.id, app); roomInfo.widgets.push(app); }); @@ -149,6 +161,13 @@ export default class WidgetStore extends AsyncStoreWithClient { public getRoomId = (widgetId: string) => { const app = this.widgetMap.get(widgetId); if (!app) return null; + + // Sanity check for https://github.com/vector-im/element-web/issues/15705 + const roomInfo = this.getRoom(app.roomId); + if (!roomInfo.widgets?.some(w => w.id === app.id)) { + throw new Error(`Widget ${app.id} says it is in ${app.roomId} but was not found there`); + } + return app.roomId; } @@ -158,7 +177,22 @@ export default class WidgetStore extends AsyncStoreWithClient { private onPinnedWidgetsChange = (settingName: string, roomId: string) => { this.initRoom(roomId); - this.getRoom(roomId).pinned = SettingsStore.getValue(settingName, roomId); + + const pinned: PinnedWidgets = SettingsStore.getValue(settingName, roomId); + + // Sanity check for https://github.com/vector-im/element-web/issues/15705 + const roomInfo = this.getRoom(roomId); + const remappedPinned: PinnedWidgets = {}; + for (const widgetId of Object.keys(pinned)) { + const isPinned = pinned[widgetId]; + if (!roomInfo.widgets?.some(w => w.id === widgetId)) { + console.warn(`Skipping pinned widget update for ${widgetId} in ${roomId} -- wrong room`); + } else { + remappedPinned[widgetId] = isPinned; + } + } + roomInfo.pinned = remappedPinned; + this.emit(roomId); this.emit(UPDATE_EVENT); };