From ea46df0d4841c8c92ed876bae7f2faa98e14da99 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 7 Jun 2021 20:19:16 -0600 Subject: [PATCH 1/3] Partially restore immutable event objects at the rendering layer This is primarily to fix some extremely rare edge cases in local echo, but also restores the accuracy of some comments in the stack regarding immutable event objects (which were made mutable many years ago). This shouldn't have any impact on the daily usage of the app, only adding a measured 0ms of latency to the stack. --- src/components/views/messages/TextualBody.js | 1 + src/components/views/rooms/EventTile.tsx | 124 +++++++++++-------- 2 files changed, 76 insertions(+), 49 deletions(-) diff --git a/src/components/views/messages/TextualBody.js b/src/components/views/messages/TextualBody.js index 3adfea6ee6..00e7d3d301 100644 --- a/src/components/views/messages/TextualBody.js +++ b/src/components/views/messages/TextualBody.js @@ -262,6 +262,7 @@ export default class TextualBody extends React.Component { // exploit that events are immutable :) return (nextProps.mxEvent.getId() !== this.props.mxEvent.getId() || + nextProps.mxEvent !== this.props.mxEvent || nextProps.highlights !== this.props.highlights || nextProps.replacingEventId !== this.props.replacingEventId || nextProps.highlightLink !== this.props.highlightLink || diff --git a/src/components/views/rooms/EventTile.tsx b/src/components/views/rooms/EventTile.tsx index 85b9cac2c4..d1b596a709 100644 --- a/src/components/views/rooms/EventTile.tsx +++ b/src/components/views/rooms/EventTile.tsx @@ -298,6 +298,9 @@ interface IState { // The Relations model from the JS SDK for reactions to `mxEvent` reactions: Relations; + // Our snapshotted/local copy of the props.mxEvent, for local echo reasons + mxEvent: MatrixEvent; + hover: boolean; } @@ -332,6 +335,8 @@ export default class EventTile extends React.Component { // The Relations model from the JS SDK for reactions to `mxEvent` reactions: this.getReactions(), + mxEvent: this.mxEvent.getSnapshotCopy(), // snapshot up front to verify it all works + hover: false, }; @@ -348,6 +353,10 @@ export default class EventTile extends React.Component { this.ref = React.createRef(); } + private get mxEvent(): MatrixEvent { + return this.state?.mxEvent ?? this.props.mxEvent; + } + /** * When true, the tile qualifies for some sort of special read receipt. This could be a 'sending' * or 'sent' receipt, for example. @@ -356,16 +365,16 @@ export default class EventTile extends React.Component { private get isEligibleForSpecialReceipt() { // First, if there are other read receipts then just short-circuit this. if (this.props.readReceipts && this.props.readReceipts.length > 0) return false; - if (!this.props.mxEvent) return false; + if (!this.mxEvent) return false; // Sanity check (should never happen, but we shouldn't explode if it does) - const room = this.context.getRoom(this.props.mxEvent.getRoomId()); + const room = this.context.getRoom(this.mxEvent.getRoomId()); if (!room) return false; // Quickly check to see if the event was sent by us. If it wasn't, it won't qualify for // special read receipts. const myUserId = MatrixClientPeg.get().getUserId(); - if (this.props.mxEvent.getSender() !== myUserId) return false; + if (this.mxEvent.getSender() !== myUserId) return false; // Finally, determine if the type is relevant to the user. This notably excludes state // events and pretty much anything that can't be sent by the composer as a message. For @@ -376,7 +385,7 @@ export default class EventTile extends React.Component { EventType.RoomMessage, EventType.RoomMessageEncrypted, ]; - if (!simpleSendableEvents.includes(this.props.mxEvent.getType())) return false; + if (!simpleSendableEvents.includes(this.mxEvent.getType())) return false; // Default case return true; @@ -418,7 +427,7 @@ export default class EventTile extends React.Component { // TODO: [REACT-WARNING] Move into constructor // eslint-disable-next-line camelcase UNSAFE_componentWillMount() { - this.verifyEvent(this.props.mxEvent); + this.verifyEvent(this.mxEvent); } componentDidMount() { @@ -448,11 +457,21 @@ export default class EventTile extends React.Component { } shouldComponentUpdate(nextProps, nextState) { + // If the echo changed meaningfully, update. + if (!this.state.mxEvent?.isEquivalentTo(nextProps.mxEvent)) { + return true; + } + if (objectHasDiff(this.state, nextState)) { return true; } - return !this.propsEqual(this.props, nextProps); + if (!this.propsEqual(this.props, nextProps)) { + return true; + } + + // Always assume there's no significant change. + return false; } componentWillUnmount() { @@ -473,11 +492,18 @@ export default class EventTile extends React.Component { this.context.on("Room.receipt", this.onRoomReceipt); this.isListeningForReceipts = true; } + + // Update the state again if the snapshot needs updating. Note that this will fire + // a second state update to re-render child components, which ultimately calls didUpdate + // again, so we break that loop with a reference check first (faster than comparing events). + if (this.state.mxEvent === prevState.mxEvent && !this.state?.mxEvent.isEquivalentTo(this.props.mxEvent)) { + this.setState({mxEvent: this.props.mxEvent.getSnapshotCopy()}); + } } private onRoomReceipt = (ev, room) => { // ignore events for other rooms - const tileRoom = MatrixClientPeg.get().getRoom(this.props.mxEvent.getRoomId()); + const tileRoom = MatrixClientPeg.get().getRoom(this.mxEvent.getRoomId()); if (room !== tileRoom) return; if (!this.shouldShowSentReceipt && !this.shouldShowSendingReceipt && !this.isListeningForReceipts) { @@ -501,19 +527,19 @@ export default class EventTile extends React.Component { // we need to re-verify the sending device. // (we call onHeightChanged in verifyEvent to handle the case where decryption // has caused a change in size of the event tile) - this.verifyEvent(this.props.mxEvent); + this.verifyEvent(this.mxEvent); this.forceUpdate(); }; private onDeviceVerificationChanged = (userId, device) => { - if (userId === this.props.mxEvent.getSender()) { - this.verifyEvent(this.props.mxEvent); + if (userId === this.mxEvent.getSender()) { + this.verifyEvent(this.mxEvent); } }; private onUserVerificationChanged = (userId, _trustStatus) => { - if (userId === this.props.mxEvent.getSender()) { - this.verifyEvent(this.props.mxEvent); + if (userId === this.mxEvent.getSender()) { + this.verifyEvent(this.mxEvent); } }; @@ -620,11 +646,11 @@ export default class EventTile extends React.Component { } shouldHighlight() { - const actions = this.context.getPushActionsForEvent(this.props.mxEvent.replacingEvent() || this.props.mxEvent); + const actions = this.context.getPushActionsForEvent(this.mxEvent.replacingEvent() || this.mxEvent); if (!actions || !actions.tweaks) { return false; } // don't show self-highlights from another of our clients - if (this.props.mxEvent.getSender() === this.context.credentials.userId) { + if (this.mxEvent.getSender() === this.context.credentials.userId) { return false; } @@ -639,7 +665,7 @@ export default class EventTile extends React.Component { getReadAvatars() { if (this.shouldShowSentReceipt || this.shouldShowSendingReceipt) { - return ; + return ; } // return early if there are no read receipts @@ -726,7 +752,7 @@ export default class EventTile extends React.Component { } onSenderProfileClick = event => { - const mxEvent = this.props.mxEvent; + const mxEvent = this.mxEvent; dis.dispatch({ action: 'insert_mention', user_id: mxEvent.getSender(), @@ -743,7 +769,7 @@ export default class EventTile extends React.Component { // Cancel any outgoing key request for this event and resend it. If a response // is received for the request with the required keys, the event could be // decrypted successfully. - this.context.cancelAndResendEventRoomKeyRequest(this.props.mxEvent); + this.context.cancelAndResendEventRoomKeyRequest(this.mxEvent); }; onPermalinkClicked = e => { @@ -752,14 +778,14 @@ export default class EventTile extends React.Component { e.preventDefault(); dis.dispatch({ action: 'view_room', - event_id: this.props.mxEvent.getId(), + event_id: this.mxEvent.getId(), highlighted: true, - room_id: this.props.mxEvent.getRoomId(), + room_id: this.mxEvent.getRoomId(), }); }; private renderE2EPadlock() { - const ev = this.props.mxEvent; + const ev = this.mxEvent; // event could not be decrypted if (ev.getContent().msgtype === 'm.bad.encrypted') { @@ -818,7 +844,7 @@ export default class EventTile extends React.Component { ) { return null; } - const eventId = this.props.mxEvent.getId(); + const eventId = this.mxEvent.getId(); return this.props.getRelationsForEvent(eventId, "m.annotation", "m.reaction"); }; @@ -837,13 +863,13 @@ export default class EventTile extends React.Component { const SenderProfile = sdk.getComponent('messages.SenderProfile'); const MemberAvatar = sdk.getComponent('avatars.MemberAvatar'); - //console.info("EventTile showUrlPreview for %s is %s", this.props.mxEvent.getId(), this.props.showUrlPreview); + //console.info("EventTile showUrlPreview for %s is %s", this.mxEvent.getId(), this.props.showUrlPreview); - const content = this.props.mxEvent.getContent(); + const content = this.mxEvent.getContent(); const msgtype = content.msgtype; - const eventType = this.props.mxEvent.getType(); + const eventType = this.mxEvent.getType(); - let tileHandler = getHandlerTile(this.props.mxEvent); + let tileHandler = getHandlerTile(this.mxEvent); // Info messages are basically information about commands processed on a room const isBubbleMessage = eventType.startsWith("m.key.verification") || @@ -860,7 +886,7 @@ export default class EventTile extends React.Component { // source tile when there's no regular tile for an event and also for // replace relations (which otherwise would display as a confusing // duplicate of the thing they are replacing). - if (SettingsStore.getValue("showHiddenEventsInTimeline") && !haveTileForEvent(this.props.mxEvent)) { + if (SettingsStore.getValue("showHiddenEventsInTimeline") && !haveTileForEvent(this.mxEvent)) { tileHandler = "messages.ViewSourceEvent"; // Reuse info message avatar and sender profile styling isInfoMessage = true; @@ -879,8 +905,8 @@ export default class EventTile extends React.Component { const EventTileType = sdk.getComponent(tileHandler); const isSending = (['sending', 'queued', 'encrypting'].indexOf(this.props.eventSendStatus) !== -1); - const isRedacted = isMessageEvent(this.props.mxEvent) && this.props.isRedacted; - const isEncryptionFailure = this.props.mxEvent.isDecryptionFailure(); + const isRedacted = isMessageEvent(this.mxEvent) && this.props.isRedacted; + const isEncryptionFailure = this.mxEvent.isDecryptionFailure(); const isEditing = !!this.props.editState; const classes = classNames({ @@ -910,14 +936,14 @@ export default class EventTile extends React.Component { let permalink = "#"; if (this.props.permalinkCreator) { - permalink = this.props.permalinkCreator.forEvent(this.props.mxEvent.getId()); + permalink = this.props.permalinkCreator.forEvent(this.mxEvent.getId()); } // we can't use local echoes as scroll tokens, because their event IDs change. // Local echos have a send "status". - const scrollToken = this.props.mxEvent.status + const scrollToken = this.mxEvent.status ? undefined - : this.props.mxEvent.getId(); + : this.mxEvent.getId(); let avatar; let sender; @@ -947,15 +973,15 @@ export default class EventTile extends React.Component { needsSenderProfile = true; } - if (this.props.mxEvent.sender && avatarSize) { + if (this.mxEvent.sender && avatarSize) { let member; // set member to receiver (target) if it is a 3PID invite // so that the correct avatar is shown as the text is // `$target accepted the invitation for $email` - if (this.props.mxEvent.getContent().third_party_invite) { - member = this.props.mxEvent.target; + if (this.mxEvent.getContent().third_party_invite) { + member = this.mxEvent.target; } else { - member = this.props.mxEvent.sender; + member = this.mxEvent.sender; } avatar = (
@@ -970,17 +996,17 @@ export default class EventTile extends React.Component { if (needsSenderProfile) { if (!this.props.tileShape || this.props.tileShape === 'reply' || this.props.tileShape === 'reply_preview') { sender = ; } else { - sender = ; + sender = ; } } const MessageActionBar = sdk.getComponent('messages.MessageActionBar'); const actionBar = !isEditing ? { onFocusChange={this.onActionBarFocusChange} /> : undefined; - const showTimestamp = this.props.mxEvent.getTs() && + const showTimestamp = this.mxEvent.getTs() && (this.props.alwaysShowTimestamps || this.props.last || this.state.hover || this.state.actionBarFocused); const timestamp = showTimestamp ? - : null; + : null; const keyRequestHelpText =
@@ -1031,7 +1057,7 @@ export default class EventTile extends React.Component { if (!isRedacted) { const ReactionsRow = sdk.getComponent('messages.ReactionsRow'); reactionsRow = ; } @@ -1039,7 +1065,7 @@ export default class EventTile extends React.Component { const linkedTimestamp = { timestamp } ; @@ -1058,7 +1084,7 @@ export default class EventTile extends React.Component { switch (this.props.tileShape) { case 'notif': { - const room = this.context.getRoom(this.props.mxEvent.getRoomId()); + const room = this.context.getRoom(this.mxEvent.getRoomId()); return React.createElement(this.props.as || "li", { "className": classes, "aria-live": ariaLive, @@ -1080,7 +1106,7 @@ export default class EventTile extends React.Component {
,
{ }, [
{ let thread; if (this.props.tileShape === 'reply_preview') { thread = ReplyThread.makeThread( - this.props.mxEvent, + this.mxEvent, this.props.onHeightChanged, this.props.permalinkCreator, this.replyThread, @@ -1148,7 +1174,7 @@ export default class EventTile extends React.Component { { groupPadlock } { thread } { } default: { const thread = ReplyThread.makeThread( - this.props.mxEvent, + this.mxEvent, this.props.onHeightChanged, this.props.permalinkCreator, this.replyThread, @@ -1188,7 +1214,7 @@ export default class EventTile extends React.Component { { groupPadlock } { thread } Date: Thu, 17 Jun 2021 14:21:01 -0600 Subject: [PATCH 2/3] Switch order --- src/components/views/messages/TextualBody.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/views/messages/TextualBody.js b/src/components/views/messages/TextualBody.js index 00e7d3d301..cb6a4f14b6 100644 --- a/src/components/views/messages/TextualBody.js +++ b/src/components/views/messages/TextualBody.js @@ -261,8 +261,8 @@ export default class TextualBody extends React.Component { //console.info("shouldComponentUpdate: ShowUrlPreview for %s is %s", this.props.mxEvent.getId(), this.props.showUrlPreview); // exploit that events are immutable :) - return (nextProps.mxEvent.getId() !== this.props.mxEvent.getId() || - nextProps.mxEvent !== this.props.mxEvent || + return (nextProps.mxEvent !== this.props.mxEvent || + nextProps.mxEvent.getId() !== this.props.mxEvent.getId() || nextProps.highlights !== this.props.highlights || nextProps.replacingEventId !== this.props.replacingEventId || nextProps.highlightLink !== this.props.highlightLink || From 98e0200b4a17fc80b1865ae25bdfec80de0bc161 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 17 Jun 2021 14:21:50 -0600 Subject: [PATCH 3/3] Function name --- src/components/views/rooms/EventTile.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/views/rooms/EventTile.tsx b/src/components/views/rooms/EventTile.tsx index d1b596a709..4dd8fff636 100644 --- a/src/components/views/rooms/EventTile.tsx +++ b/src/components/views/rooms/EventTile.tsx @@ -335,7 +335,7 @@ export default class EventTile extends React.Component { // The Relations model from the JS SDK for reactions to `mxEvent` reactions: this.getReactions(), - mxEvent: this.mxEvent.getSnapshotCopy(), // snapshot up front to verify it all works + mxEvent: this.mxEvent.toSnapshot(), // snapshot up front to verify it all works hover: false, }; @@ -497,7 +497,7 @@ export default class EventTile extends React.Component { // a second state update to re-render child components, which ultimately calls didUpdate // again, so we break that loop with a reference check first (faster than comparing events). if (this.state.mxEvent === prevState.mxEvent && !this.state?.mxEvent.isEquivalentTo(this.props.mxEvent)) { - this.setState({mxEvent: this.props.mxEvent.getSnapshotCopy()}); + this.setState({mxEvent: this.props.mxEvent.toSnapshot()}); } }