-
-
Notifications
You must be signed in to change notification settings - Fork 618
Draft: Refresh room timeline when we see a MSC2716 marker event #2282
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
MadLittleMods
wants to merge
21
commits into
develop
from
madlittlemods/refresh-timeline-when-we-see-msc2716-marker-events
Closed
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
2b029c9
Refresh room timeline when we see a MSC2716 marker event
MadLittleMods ffbcbff
Don't refresh timeline when first sync or from cache
MadLittleMods 39cd6e9
Fix docstring typo
MadLittleMods e5f34db
Only notify client that timeline should be refreshed instead of doing…
MadLittleMods e070b3c
Remove console.log
MadLittleMods 21f6aa2
Fixup docstrings
MadLittleMods 9bfcf4b
Fix some lints
MadLittleMods 1c2b955
Merge branch 'develop' into madlittlemods/refresh-timeline-when-we-se…
MadLittleMods 05b9800
Fix some lints
MadLittleMods 8b82bb5
Rename to roomState to align with other options objects
MadLittleMods 35f3f04
Rename to toStartOfTimeline to align with other options objects
MadLittleMods b2636c3
Revert arg rename change for unused
MadLittleMods da57a74
Fix up tests
MadLittleMods c539b64
Fix timeline-window specs
MadLittleMods 11040e5
Stop prompting to refresh timeline when syncing from cache
MadLittleMods 07fcf27
Fix lints and move to logger
MadLittleMods 701bf34
Remove todo
MadLittleMods c080c6c
Remove fixme in favor of aspirational explanation of what we could do…
MadLittleMods 0694b84
Add some explanation comments
MadLittleMods 26fb2b6
Add some tests
MadLittleMods 8ca2645
WIP: messy timelineWasEmpty
MadLittleMods File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would probably be better if we re-fetched the new events first, then replaced the timeline
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theinsertionEventId
the marker is pointing to and check if any itsprev_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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to prompt the user to refresh their timeline: matrix-org/matrix-react-sdk#8303
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 theprev_events
.