Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix spurious notifications #1339

Merged
merged 2 commits into from
Aug 29, 2017
Merged

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Aug 25, 2017

When loading new content whilst scrolling down the timeline .

Use the Event event which only fire for events received in a sync,
rather than Room.timeline which fires for events from pagination
too.

Fixes element-hq/element-web#2608
Fixes element-hq/element-web#1603

When loading new content whilst scrolling down the timeline .

Use the Event event which only fire for events received in a sync,
rather than Room.timeline which fires for events from pagination
too.
@lukebarnard1
Copy link
Contributor

This looks like it could inadvertently be affecting the logic for dealing with an event that's still being decrypted or has failed to be decrypted.

@dbkr
Copy link
Member Author

dbkr commented Aug 25, 2017

we'll necessarily get Event before Event.decrypted so it shouldn't do? I tested it and it worked. Was there a particular failure mode you thought of?

@lukebarnard1
Copy link
Contributor

lukebarnard1 commented Aug 25, 2017

Nothing in particular, I'm just applying paranoia. Are events even being decrypted at the point Event gets fired? and should we be running that logic when Event is received as opposed to Room.Timeline?

@dbkr
Copy link
Member Author

dbkr commented Aug 25, 2017

I don't think the js-sdk makes any particular guarantees if the event is decrypted when you get the Event event or some time after, which is why we check if it's decrypted at the time of getting Event and if it's not, wait until it is. Room.Timeline fires for all events that go into the timeline, including paginated ones, hence moving it here.

@lukebarnard1
Copy link
Contributor

lukebarnard1 commented Aug 25, 2017

Right so now we won't be running this logic - including the bit for events potentially still being decrypted - when we paginate. This seems like it would cause different results for an event still being decrypted during pagination.

@dbkr
Copy link
Member Author

dbkr commented Aug 25, 2017

The point is that we should never be notifying for events received through pagination.

@lukebarnard1
Copy link
Contributor

Yes but do we not want encrypted events to be added to pendingEncryptedEventIds when paginating?

@dbkr
Copy link
Member Author

dbkr commented Aug 29, 2017

No, Notifier's job is just showing notifications, so it doesn't care about events we won't notify for.

@lukebarnard1
Copy link
Contributor

Oh 🙂 that's fine then

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants