Draft: Refresh room timeline when we see a MSC2716 marker event #2282

MadLittleMods · 2022-04-08T03:19:07Z

Refresh room timeline when we see a MSC2716 marker event.

The "marker" events are being used as state now because this way they can't be lost in the timeline. Regardless of when they were sent, we will still have the latest version of the state to compare against. Any time we see our latest state value change for marker events, refresh the timeline.

In a sync meeting with @ara4n, we came up with the idea to make the marker events as state events. When the client sees that the m.room.marker state changed to a different event ID, it can throw away all of the timeline and re-fetch as needed.

For homeservers where the same problem can happen, we probably don't want to throw away the whole timeline but it can go up the unsigned.replaces_state chain of the m.room.marker state events to get them all.

In terms of state performance, there could be thousands of marker events in a room but it's no different than room members joining and leaving over and over like an IRC room.

-- matrix-org/matrix-spec-proposals#2716 (comment)

Todo

Add tests
- When already joined to room, new marker event will cause timelineNeedsRefresh
- marker event already sent in room, user joins room where the /sync state includes marker should NOT trigger timelineNeedsRefresh
- When already joined to room, marker event already sent in room, initial /sync state includes marker should NOT trigger timelineNeedsRefresh
- When syncFromCache where the state includes marker should NOT trigger timelineNeedsRefresh
- In normal room version, marker from creator will cause timelineNeedsRefresh
- In normal room version, marker not from creator should NOT trigger timelineNeedsRefresh
Add method to refresh the timeline to use in Ask to refresh timeline when historical messages are imported (MSC2716) matrix-react-sdk#8303 (comment)

Dev notes

yarn jest spec/integ/matrix-client-syncing.spec.js --silent=false --verbose=false

        Error.stackTraceLimit = Infinity;
        console.log(`setStateEvents timelineWasEmpty=${markerFoundOptions && markerFoundOptions.timelineWasEmpty} stateEvents:\n`, stateEvents.map((ev) => {
            return `\t${ev.getType()} ${ev.getId()}`;
        }).join('\n'), new Error().stack);

room.resetLiveTimeline(null, null);

addEventsToTimeline
addEventToTimeline
addLiveEvent

resetLiveTimeline

resetAllTimelines

removeFilteredTimelineSet


syncFromCache
processSyncResponse
processRoomEvents
room.addLiveEvents


sessionStore


processSyncResponse
processRoomEvents
initialiseState
setStateEvents

load
getEventTimeline
initialiseState
setStateEvents

Here's what your changelog entry will look like:

✨ Features

Draft: Refresh room timeline when we see a MSC2716 marker event (#2282). Contributed by @MadLittleMods.

@ara4n

See matrix-org/matrix-spec-proposals#2716 (comment) > In a [sync meeting with @ara4n](https://docs.google.com/document/d/1KCEmpnGr4J-I8EeaVQ8QJZKBDu53ViI7V62y5BzfXr0/edit#bookmark=id.67nio1ka8znc), we came up with the idea to make the `marker` events as state events. When the client sees that the `m.room.marker` state changed to a different event ID, it can throw away all of the timeline and re-fetch as needed. > > For homeservers where the [same problem](matrix-org/matrix-spec-proposals#2716 (comment)) can happen, we probably don't want to throw away the whole timeline but it can go up the `unsigned.replaces_state` chain of the `m.room.marker` state events to get them all. > > In terms of state performance, there could be thousands of `marker` events in a room but it's no different than room members joining and leaving over and over like an IRC room.

src/sync.ts

Context: #2282 (comment)

MadLittleMods · 2022-04-08T06:42:03Z

src/sync.ts

+            ) {
+                // Saw new marker event, refreshing the timeline
+                console.log('Saw new marker event, refreshing the timeline', new Error().stack);
+                room.resetLiveTimeline(null, null);


TODO: Test if this is the function we actually want to use.

Our goal is to throw away the current timeline and re-fetch events for the room from the server.

It would probably be better if we re-fetched the new events first, then replaced the timeline

Thinking about this more, do we even want to do this automatically? Someone could cause your Element client to constantly refresh the timeline by just sending marker events over and over. Granted, you probably want to leave a room like this 🤷. Perhaps also some sort of DOS vector since everyone will be refreshing and hitting the server at the exact same time.

I think we might want to approach this to be a similar flow to how Element receives app updates. When the client receives a marker event, we first notify the user that history was imported and is available to see after they refresh their timeline (which can be a button action). The UI can probably use something similar to the "Connectivity to the server has been lost." banner.

In most cases, the history won't be in people's current timelines cached in the Element client, so do we even need to worry about refreshing? This is what I kinda leaned to before this PR but it's slightly important from a demo perspective and a personal bridge flow. For example with Beeper, a room is created for your Whatsapp conversation, and as you sit in the room, you probably ideally want the history to appear as it is imported but at least be apparent in some way after the import finishes.

We can detect whether the history imported affects any timelines on the client and only ask to refresh then. To detect, when we receive a marker event, make a request to /context/{insertionEventId} with the insertionEventId the marker is pointing to and check if any its prev_events are in the timeline. /context doesn't work over federation but the homeserver backfills the insertion event when it receives the marker so it already has it available.

Updated to prompt the user to refresh their timeline: matrix-org/matrix-react-sdk#8303

We can detect whether the history imported affects any timelines on the client and only ask to refresh then. To detect, when we receive a marker event, make a request to /context/{insertionEventId} with the insertionEventId the marker is pointing to and check if any its prev_events are in the timeline. /context doesn't work over federation but the homeserver backfills the insertion event when it receives the marker so it already has it available.

We can't do this because prev_events aren't available in the client API's 😢. I'm not too keen to add an extra field to the marker or insertion event indicating where the history is imported next to as there isn't a way to enforce that it's true (a homeserver could lie and make it up). I'd rather keep the source of truth in the prev_events.

turt2live

review to clear codeowners. When leaving draft it'll need to be re-reviewed.

src/models/event-timeline-set.ts

Associated matrix-js-sdk PR: matrix-org/matrix-js-sdk#2282

… it automatically See #2282 (comment)

MadLittleMods · 2022-04-13T01:55:32Z

src/models/event-timeline-set.ts

+     *  2. It's the first state we're seeing after joining the room
+     *  3. Or whether it's coming from `syncFromCache` */
+    fromInitialState?: boolean;
+}


Instead of dealing with unwieldy long function signatures with unlabeled and optional arguments in the usage, I've refactored some of these into option objects now that I am adding yet another boolean here (adding fromInitialState).

Before:

room.addLiveEvents(mainTimelineEvents, null, fromCache, timelineWasEmpty);

After:

room.addLiveEvents(mainTimelineEvents, { fromCache, fromInitialState: timelineWasEmpty });

If necessary, this refactor can be split out to another PR with some effort.

…msc2716-marker-events Conflicts: src/models/room.ts src/sync.ts

src/sync.ts

… better

codecov-commenter · 2022-04-13T20:45:54Z

Codecov Report

Merging #2282 (0694b84) into develop (9aab917) will decrease coverage by 0.09%.
The diff coverage is 46.57%.

❗ Current head 0694b84 differs from pull request most recent head 26fb2b6. Consider uploading reports for the commit 26fb2b6 to get more accurate results

@@             Coverage Diff             @@
##           develop    #2282      +/-   ##
===========================================
- Coverage    59.71%   59.62%   -0.10%     
===========================================
  Files           91       91              
  Lines        16439    16484      +45     
  Branches      3794     3808      +14     
===========================================
+ Hits          9817     9828      +11     
- Misses        6622     6656      +34

Impacted Files	Coverage Δ
src/@types/event.ts	`100.00% <ø> (ø)`
src/client.ts	`38.04% <ø> (ø)`
src/models/thread.ts	`74.10% <ø> (ø)`
src/sync.ts	`63.85% <17.39%> (-1.73%)`	⬇️
src/models/room.ts	`56.71% <29.16%> (-0.63%)`	⬇️
src/models/room-state.ts	`77.25% <66.66%> (-0.16%)`	⬇️
src/models/event-timeline-set.ts	`79.38% <81.81%> (+0.23%)`	⬆️
src/models/event-timeline.ts	`93.75% <100.00%> (+0.05%)`	⬆️

MadLittleMods · 2022-04-13T21:06:44Z

src/sync.ts

+        room.currentState.on(RoomStateEvent.Marker, async function(
+            markerEvent,
+            { fromInitialState }: ISetStateOptions = {},
+        ) {


Calling out one of the main points of interest in the PR (new logic)

MadLittleMods · 2022-04-13T21:08:27Z

src/models/room-state.ts

@@ -401,6 +414,8 @@ export class RoomState extends TypedEventEmitter<EmittedEvents, EventHandlerMap>

                // assume all our sentinels are now out-of-date
                this.sentinels = {};
+            } else if (event.getType() === EventType.Marker) {
+                this.emit(RoomStateEvent.Marker, event, setStateOptions);


When we see a new marker state event, emit an event so we can listen for it in sync.ts -> registerStateListeners and see whether we need to refresh the timeline

MadLittleMods · 2022-04-14T06:16:55Z

Closing in favor of #2299

MadLittleMods requested a review from a team as a code owner April 8, 2022 03:19

MadLittleMods marked this pull request as draft April 8, 2022 03:19

MadLittleMods commented Apr 8, 2022

View reviewed changes

src/sync.ts Show resolved Hide resolved

Don't refresh timeline when first sync or from cache

ffbcbff

Context: #2282 (comment)

MadLittleMods commented Apr 8, 2022

View reviewed changes

MadLittleMods added the T-Enhancement label Apr 8, 2022

turt2live reviewed Apr 8, 2022

View reviewed changes

Fix docstring typo

39cd6e9

MadLittleMods commented Apr 9, 2022

View reviewed changes

src/models/event-timeline-set.ts Show resolved Hide resolved

MadLittleMods added a commit to matrix-org/matrix-react-sdk that referenced this pull request Apr 13, 2022

Ask to refresh timeline when historical messages are imported (MSC2716)

66ea757

Associated matrix-js-sdk PR: matrix-org/matrix-js-sdk#2282

MadLittleMods mentioned this pull request Apr 13, 2022

Ask to refresh timeline when historical messages are imported (MSC2716) matrix-org/matrix-react-sdk#8303

Closed

2 tasks

MadLittleMods added 3 commits April 12, 2022 20:10

Only notify client that timeline should be refreshed instead of doing…

e5f34db

… it automatically See #2282 (comment)

Remove console.log

e070b3c

Fixup docstrings

21f6aa2

MadLittleMods commented Apr 13, 2022

View reviewed changes

MadLittleMods added 10 commits April 12, 2022 21:15

Fix some lints

9bfcf4b

Merge branch 'develop' into madlittlemods/refresh-timeline-when-we-see-…

1c2b955

…msc2716-marker-events Conflicts: src/models/room.ts src/sync.ts

Fix some lints

05b9800

Rename to roomState to align with other options objects

8b82bb5

Rename to toStartOfTimeline to align with other options objects

35f3f04

Revert arg rename change for unused

b2636c3

Fix up tests

da57a74

Fix timeline-window specs

c539b64

Stop prompting to refresh timeline when syncing from cache

11040e5

Fix lints and move to logger

07fcf27

MadLittleMods commented Apr 13, 2022

View reviewed changes

src/sync.ts Outdated Show resolved Hide resolved

MadLittleMods added 2 commits April 13, 2022 15:39

Remove todo

701bf34

Remove fixme in favor of aspirational explanation of what we could do…

c080c6c

… better

Add some explanation comments

0694b84

MadLittleMods commented Apr 13, 2022

View reviewed changes

MadLittleMods added 2 commits April 13, 2022 18:58

Add some tests

26fb2b6

WIP: messy timelineWasEmpty

8ca2645

MadLittleMods closed this Apr 14, 2022

MadLittleMods mentioned this pull request Apr 14, 2022

Timeline needs to refresh when we see a MSC2716 marker event #2299

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft: Refresh room timeline when we see a MSC2716 marker event #2282

Draft: Refresh room timeline when we see a MSC2716 marker event #2282

MadLittleMods commented Apr 8, 2022 •

edited by github-actions bot

Loading

MadLittleMods Apr 8, 2022

MadLittleMods Apr 8, 2022

MadLittleMods Apr 8, 2022 •

edited

Loading

MadLittleMods Apr 13, 2022

turt2live left a comment

MadLittleMods Apr 13, 2022 •

edited

Loading

codecov-commenter commented Apr 13, 2022 •

edited

Loading

MadLittleMods Apr 13, 2022 •

edited

Loading

MadLittleMods Apr 13, 2022

MadLittleMods commented Apr 14, 2022

Draft: Refresh room timeline when we see a MSC2716 marker event #2282

Draft: Refresh room timeline when we see a MSC2716 marker event #2282

Conversation

MadLittleMods commented Apr 8, 2022 • edited by github-actions bot Loading

Todo

Dev notes

✨ Features

MadLittleMods Apr 8, 2022

Choose a reason for hiding this comment

MadLittleMods Apr 8, 2022

Choose a reason for hiding this comment

MadLittleMods Apr 8, 2022 • edited Loading

Choose a reason for hiding this comment

MadLittleMods Apr 13, 2022

Choose a reason for hiding this comment

turt2live left a comment

Choose a reason for hiding this comment

MadLittleMods Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Apr 13, 2022 • edited Loading

Codecov Report

MadLittleMods Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

MadLittleMods Apr 13, 2022

Choose a reason for hiding this comment

MadLittleMods commented Apr 14, 2022

MadLittleMods commented Apr 8, 2022 •

edited by github-actions bot

Loading

MadLittleMods Apr 8, 2022 •

edited

Loading

MadLittleMods Apr 13, 2022 •

edited

Loading

codecov-commenter commented Apr 13, 2022 •

edited

Loading

MadLittleMods Apr 13, 2022 •

edited

Loading