Hide pre-join UTDs #3881

uhoreg · 2020-01-20T21:58:11Z

When a user joins an encrypted room, any undecryptable messages before the join are hidden. (More precisely, any events before the most recent undecryptable message before the join.)

(I plan on squash-merging this, since the commit history is a bit convoluted.)

uhoreg · 2020-01-23T19:39:31Z

fixes element-hq/element-web#5642

foldleft · 2020-01-24T11:03:40Z

I think I like this, but I'm going to leave this to Dave or someone who understands event/DAG structure a little better to approve.

A nit, or a suggestion, or, only bother touching this if you happen to agree – I found it took me a moment to understand the logic where you're looking for a timeline. It might be more readable to run a tight loop to find the timeline, then do the stuff in the if/break block afterwards?

jryans

Hmm, it's probably okay overall, but I'd very much like to avoid O(n^2) work here if at all possible.

Also curious if there are opportunities to break out earlier in the process... Wouldn't it be faster for the happy path (where everything is visible) if you instead go forward through the events? The first time you see a decryptable message, you can stop, and in the happy path, that's the first one, so there's almost nothing to do, right? (I have probably missed something important here... 😅)

src/components/structures/MessagePanel.js

uhoreg · 2020-01-24T17:19:50Z

It's not O(n^2) work. Although I have a loop within a loop, the inner loop only runs once. On the other hand, I'm not too happy about running the loop every time render is called -- I don't know how often that happens in React world -- so maybe we can pull it to somewhere better? Suggestions welcome.

Regarding only running the loop when we find an undecryptable message, we could do that, but then we'd be running through all the events anyways, so I don't think that buys us much.

jryans

Overall, I think this would be more naturally accomplished in TimelinePanel. However, this is a tricky area to work in, so I'll also flag @dbkr to ensure I am not leading you astray.

src/components/structures/MessagePanel.js

dbkr · 2020-01-27T14:36:35Z

Yep, although we also do things like MemberEventListSummary on render in MessagePanel so there is some precedent for this, for better or worse.

uhoreg · 2020-01-27T21:42:13Z

src/components/structures/TimelinePanel.js

+                const prevContent = event.getPrevContent();
+                userMembership = prevContent.membership || "leave";
+            } else if (userMembership === "leave" &&
+                       (event.isDecryptionFailure() || event.isBeingDecrypted())) {


One result of moving this to TimelinePanel is that I had to make it also hide things that are in the process of being decrypted (which is probably a wrong thing to do), rather than just things that failed to decrypt, presumably because it hasn't had enough time to finish trying to decrypt. It would be nice if there was a better way to do this, but I can't see anything better.

Hmm, not sure I follow why that's only an issue up here in TimelinePanel, I would have assumed it would apply in either component...

What is the user experience with and without including events being decrypted here?

I'm not entirely sure why it only happens when it's in TimelinePanel and not in MessagePanel. My best guess is that it's a timing issue -- by the time MessagePanel gets called, the message decryption has completed, whereas in TimelinePanel, it's still trying to decrypt.

If I don't include isBeingDecrypted, then it will just show all the events, without dropping the pre-join UISIs.

jryans · 2020-01-27T21:48:06Z

src/components/structures/TimelinePanel.js

+        // timeline if they have not been sent yet) and get the user's membership
+        // at that point.
+        for (i = events.length - 1; i >= 0; i--) {
+            const timeline = room.getTimelineForEvent(events[i].getId());


Hmm, I was hoping that moving up to TimelinePanel would also allow removing the outer loop and timeline check by only looking at liveEvents in state (instead of plain events). Is it possible to make that simplification, or does it break down somewhere?

You're right, we don't need to use events. I've changed it to use liveEvents instead of events. (liveEvents vs events confused me.) I'm not sure if the outer loop is needed, but if it isn't, then it will exit after the first iteration of the loop.

liveEvents is the set we know are already in a timeline:

matrix-react-sdk/src/components/structures/TimelinePanel.js

Lines 1116 to 1120 in 380a81b

const events = this._timelineWindow.getEvents();

// Hold onto the live events separately. The read receipt and read marker

// should use this list, so that they don't advance into pending events.

const liveEvents = [...events];

events is liveEvents plus the pending / unsent events as well. (Maybe not the best naming, open to suggestions...!) 😅

Given that then, let's remove the outer loop and update the comment since we know the last event of liveEvents is always the one we'll pick. (This is a somewhat complex function, so I'd like to simplify it where possible like this.)

jryans

Thanks for going ahead with the move, overall I think it's easier to follow here. 😄 A few more comments before landing.

jryans · 2020-01-28T10:20:52Z

src/components/structures/TimelinePanel.js

+        // timeline if they have not been sent yet) and get the user's membership
+        // at that point.
+        for (i = events.length - 1; i >= 0; i--) {
+            const timeline = room.getTimelineForEvent(events[i].getId());


liveEvents is the set we know are already in a timeline:

matrix-react-sdk/src/components/structures/TimelinePanel.js

Lines 1116 to 1120 in 380a81b

const events = this._timelineWindow.getEvents();

// Hold onto the live events separately. The read receipt and read marker

// should use this list, so that they don't advance into pending events.

const liveEvents = [...events];

events is liveEvents plus the pending / unsent events as well. (Maybe not the best naming, open to suggestions...!) 😅

Given that then, let's remove the outer loop and update the comment since we know the last event of liveEvents is always the one we'll pick. (This is a somewhat complex function, so I'd like to simplify it where possible like this.)

jryans · 2020-01-28T10:27:18Z

src/components/structures/TimelinePanel.js

@@ -330,10 +333,12 @@ const TimelinePanel = createReactClass({
            // We can now paginate in the unpaginated direction
            const canPaginateKey = (backwards) ? 'canBackPaginate' : 'canForwardPaginate';
            const { events, liveEvents } = this._getEvents();
+            const firstVisibleEventIndex = this._checkForPreJoinUISI(liveEvents);


It makes sense that we'd need to run this every time we get events, so I think it would be more natural to call _checkForPreJoinUISI inside _getEvents, and then firstVisibleEventIndex would be another key returned by _getEvents.

jryans · 2020-01-28T11:57:56Z

src/components/structures/TimelinePanel.js

@@ -390,7 +397,7 @@ const TimelinePanel = createReactClass({
            // itself into the right place
            return new Promise((resolve) => {
                this.setState(newState, () => {
-                    resolve(r);
+                    resolve(r && (!backwards || firstVisibleEventIndex === 0));


Please add a comment to explain this.

jryans · 2020-01-28T12:01:02Z

src/components/structures/TimelinePanel.js

+                const prevContent = event.getPrevContent();
+                userMembership = prevContent.membership || "leave";
+            } else if (userMembership === "leave" &&
+                       (event.isDecryptionFailure() || event.isBeingDecrypted())) {


Hmm, not sure I follow why that's only an issue up here in TimelinePanel, I would have assumed it would apply in either component...

What is the user experience with and without including events being decrypted here?

jryans

Great, thanks for working on this, I think it's looking good to go. 😄

jryans · 2020-01-28T17:39:40Z

@uhoreg, for the test issues, it looks maybe related to decorator changes on develop...? I'd suggest merging the latest changes from develop into your branch.

uhoreg added 10 commits January 20, 2020 16:47

initial attempt to hide pre-join UTDs

f16090c

lint

361add0

move UTD hiding to message panel

3743052

lint

fcfee87

unbreak it

feb36ac

use a separate component for warning message

8e0fe2a

lint

bfca241

lint and use correct constant

e3e89e2

just hide pre-join UTDs, as per feedback from Nad, and other fixes

1bebba9

fix jsdoc and i18n

f1a0430

uhoreg requested a review from a team January 23, 2020 19:00

uhoreg marked this pull request as ready for review January 23, 2020 19:00

uhoreg mentioned this pull request Jan 23, 2020

UISI mitigation (uhoreg's issues) element-hq/riot-meta#314

Open

18 tasks

jryans requested changes Jan 24, 2020

View reviewed changes

src/components/structures/MessagePanel.js Outdated Show resolved Hide resolved

use the right index for the loop

63a56f9

uhoreg requested a review from jryans January 24, 2020 19:10

jryans requested changes Jan 24, 2020

View reviewed changes

jryans requested a review from dbkr January 24, 2020 21:29

uhoreg added 2 commits January 27, 2020 16:12

move logic (back) to TimelinePanel

a41bb69

remove changes from TimelinePanel and update copyright of MessagePanel

ee7988e

uhoreg commented Jan 27, 2020

View reviewed changes

uhoreg requested a review from jryans January 27, 2020 21:42

jryans reviewed Jan 27, 2020

View reviewed changes

use liveEvents instead of events

380a81b

uhoreg requested a review from jryans January 28, 2020 03:10

jryans requested changes Jan 28, 2020

View reviewed changes

apply changes from PR review

187f1f4

uhoreg requested a review from jryans January 28, 2020 16:53

jryans approved these changes Jan 28, 2020

View reviewed changes

Merge branch 'develop' into uhoreg/hide_pre_join_utds

a37df92

uhoreg merged commit 793ff2c into develop Jan 28, 2020

This was referenced Jan 28, 2020

Improve the UX for messages that cannot be decrypted element-hq/element-web#5642

Closed

Check for timeline in pre-join UISI path #3972

Merged

uhoreg mentioned this pull request Apr 16, 2021

Room history sometimes not visible even when historical room keys are shared. element-hq/element-web#16983

Closed

t3chguy mentioned this pull request Oct 19, 2021

Permalinks behave unreliably when target hidden by pre-join-UISI-hider element-hq/element-web#19230

Open

aaronraimist mentioned this pull request Nov 4, 2021

Hide undecryptable messages which predate your device's participation in a e2e room element-hq/element-web#2258

Closed

t3chguy mentioned this pull request Jan 24, 2022

Cannot scroll to the beginning of an encrypted room element-hq/element-web#20658

Closed

bradtgmurray mentioned this pull request Sep 30, 2022

Scrolling: Ability to scrollback can get stuck element-hq/element-web#18325

Closed

This was referenced Jan 9, 2023

Unable To Decrypt meta issue element-hq/element-meta#245

Open

A single pre-join UISI hides the whole of history element-hq/element-web#24190

Closed

This was referenced Feb 28, 2024

Epic: Users should not see UTDs for messages they are not supposed to be able to read element-hq/element-meta#2312

Open

Expected UTDs: Hide UTDs for messages sent while you were not in the room ("Pre-join UTDs") element-hq/element-meta#2317

Open

uhoreg mentioned this pull request Apr 30, 2024

Remove old pre-join UTD logic #12464

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide pre-join UTDs #3881

Hide pre-join UTDs #3881

uhoreg commented Jan 20, 2020 •

edited

uhoreg commented Jan 23, 2020

foldleft commented Jan 24, 2020 •

edited

jryans left a comment

uhoreg commented Jan 24, 2020

jryans left a comment

dbkr commented Jan 27, 2020

uhoreg Jan 27, 2020

jryans Jan 28, 2020

uhoreg Jan 28, 2020

jryans Jan 27, 2020

uhoreg Jan 28, 2020

jryans Jan 28, 2020

jryans left a comment

jryans Jan 28, 2020

jryans Jan 28, 2020

jryans Jan 28, 2020

jryans Jan 28, 2020

jryans left a comment

jryans commented Jan 28, 2020

	const events = this._timelineWindow.getEvents();

	// Hold onto the live events separately. The read receipt and read marker
	// should use this list, so that they don't advance into pending events.
	const liveEvents = [...events];

Hide pre-join UTDs #3881

Hide pre-join UTDs #3881

Conversation

uhoreg commented Jan 20, 2020 • edited

uhoreg commented Jan 23, 2020

foldleft commented Jan 24, 2020 • edited

jryans left a comment

Choose a reason for hiding this comment

uhoreg commented Jan 24, 2020

jryans left a comment

Choose a reason for hiding this comment

dbkr commented Jan 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jryans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jryans left a comment

Choose a reason for hiding this comment

jryans commented Jan 28, 2020

uhoreg commented Jan 20, 2020 •

edited

foldleft commented Jan 24, 2020 •

edited