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

MadLittleMods · 2022-04-13T01:08:22Z

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

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

Beware: edge case

There is one edge case where the "History import detected" banner can re-appear after you have refreshed the timeline. matrix-js-sdk only persists /sync data to IndexDB every 5 minutes which means it can have a stale next_batch pagination token compared to the latest events you might be seeing on screen. So if you receive a marker event, it will show the banner to refresh the timeline. You can then use the button to refresh your timeline and the banner will disappear as expected. Then if you restart your entire Element client (like refreshing the entire browser), Element will syncFromCache with the /sync response from 5 minutes ago and query /sync with that old pagination token which will fetch the marker event again and show the banner again.

Dev notes

loadTimeline
refreshTimeline
onRoomTimelineReset

onRoomTimeline
onMessageListFillRequest
onMessageListUnfillRequest

Todo

Add tests
Integrate method from matrix-js-sdk to actually refresh the timeline, Ask to refresh timeline when historical messages are imported (MSC2716) #8303 (comment)

Here's what your changelog entry will look like:

✨ Features

Ask to refresh timeline when historical messages are imported (MSC2716) (#8303). Contributed by @MadLittleMods.

Preview: https://pr8303--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

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

MadLittleMods · 2022-04-13T01:48:41Z

src/components/structures/RoomStatusBar.tsx

+        // TODO: What's the best way to refresh the timeline? Something like
+        // `room.resetLiveTimeline(null, null);` although this just seems to
+        // clear the timeline. I also tried to split out
+        // `scrollbackFromPaginationToken` from the `scrollback` method in to
+        // paginate from the beginning of the room but it's just not right.


What's the best way to refresh the timeline? Something like room.resetLiveTimeline(null, null); although this just seems to clear the timeline.

I also tried to split out the logic from scrollback into a new scrollbackFromPaginationToken method in order to paginate from the latest in the room (instead of from the current paginationToken) and fill in the timeline again after it goes blank but the room just stays blank.

scrollbackFromPaginationToken refactor

/** * Retrieve older messages from the given room and put them in the timeline. * * If this is called multiple times whilst a request is ongoing, the <i>same</i> * Promise will be returned. If there was a problem requesting scrollback, there * will be a small delay before another request can be made (to prevent tight-looping * when there is no connection). * * @param {Room} room The room to get older messages in. * @param {Integer} limit Optional. The maximum number of previous events to * pull in. Default: 30. * @param {module:client.callback} callback Optional. * @return {Promise} Resolves: Room. If you are at the beginning * of the timeline, <code>Room.oldState.paginationToken</code> will be * <code>null</code>. * @return {module:http-api.MatrixError} Rejects: with an error response. */ public scrollback(room: Room, limit = 30, callback?: Callback): Promise<Room> { if (utils.isFunction(limit)) { callback = limit as any as Callback; // legacy limit = undefined; } let timeToWaitMs = 0; let info = this.ongoingScrollbacks[room.roomId] || {}; if (info.promise) { return info.promise; } else if (info.errorTs) { const timeWaitedMs = Date.now() - info.errorTs; timeToWaitMs = Math.max(SCROLLBACK_DELAY_MS - timeWaitedMs, 0); } if (room.oldState.paginationToken === null) { return Promise.resolve(room); // already at the start. } // attempt to grab more events from the store first const numAdded = this.store.scrollback(room, limit).length; if (numAdded === limit) { // store contained everything we needed. return Promise.resolve(room); } // reduce the required number of events appropriately limit = limit - numAdded; const prom = Promise.resolve().then(async () => { try { // wait for a time before doing this request // (which may be 0 in order not to special case the code paths) await sleep(timeToWaitMs) await this.scrollbackFromPaginationToken({ room, fromToken: room.oldState.paginationToken, direction: Direction.Backward, limit, }) this.ongoingScrollbacks[room.roomId] = null; callback?.(null, room); return room; } catch(err) { this.ongoingScrollbacks[room.roomId] = { errorTs: Date.now(), }; callback?.(err); throw err; } }); info = { promise: prom, errorTs: null, }; this.ongoingScrollbacks[room.roomId] = info; return prom; } public async scrollbackFromPaginationToken({ room, fromToken, direction, limit, }: { room: Room, fromToken: string | null, direction: Direction, limit?: number, }) { const res: IMessagesResponse = await this.createMessagesRequest( room.roomId, fromToken, limit, direction, ); const matrixEvents = res.chunk.map(this.getEventMapper()); if (res.state) { const stateEvents = res.state.map(this.getEventMapper()); room.currentState.setUnknownStateEvents(stateEvents); } const [timelineEvents, threadedEvents] = this.partitionThreadedEvents(room, matrixEvents); room.addEventsToTimeline(timelineEvents, true, room.getLiveTimeline()); await this.processThreadEvents(room, threadedEvents, true); room.oldState.paginationToken = res.end; if (res.chunk.length === 0) { room.oldState.paginationToken = null; } this.store.storeEvents(room, matrixEvents, res.end, true); }

Assigned review to get some advice for this question ^

Otherwise just need to add tests for this PR

turt2live

clearing code review - will need to re-request when not-draft.

…oom when the marker was sent

codecov · 2022-04-13T07:55:09Z

Codecov Report

Merging #8303 (ed910bb) into develop (a4d3da7) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

❗ Current head ed910bb differs from pull request most recent head 8d61226. Consider uploading reports for the commit 8d61226 to get more accurate results

@@             Coverage Diff             @@
##           develop    #8303      +/-   ##
===========================================
- Coverage    29.79%   29.78%   -0.01%     
===========================================
  Files          872      872              
  Lines        50006    50019      +13     
  Branches     12723    12728       +5     
===========================================
  Hits         14897    14897              
- Misses       35109    35122      +13

Impacted Files	Coverage Δ
src/components/structures/FilePanel.tsx	`1.12% <0.00%> (ø)`
src/components/structures/RoomStatusBar.tsx	`6.93% <0.00%> (-1.03%)`	⬇️
src/indexing/EventIndex.ts	`0.59% <0.00%> (ø)`

t3chguy · 2022-04-14T08:02:34Z

src/components/structures/RoomStatusBar.tsx

        const client = this.context;
        client.on("sync", this.onSyncStateChange);
        client.on("Room.localEchoUpdated", this.onRoomLocalEchoUpdated);
+        client.on("Room.historyImportedWithinTimeline", this.onRoomHistoryImportedWithinTimeline);


Please add the context type definition around line L87 as string emitter keys are no longer allowed in TypedEventEmitter, TS thinks this.context is any here though

Can we also not listen on the room directly, re-emitters suck

Can we also not listen on the room directly, re-emitters suck

So I don't guess, what should we do? 🙇

this.props.room.on(...); but with componentDidUpdate comparing this.props.room with prevProps.room to remove old listener and setup a new one. This is all a lot nicer in hooks where its a one-liner.

this.props.room.on(...); but with componentDidUpdate comparing this.props.room with prevProps.room to remove old listener and setup a new one. This is all a lot nicer in hooks where its a one-liner.

Updated 👍

Please add the context type definition around line L87 as string emitter keys are no longer allowed in TypedEventEmitter

I'm not sure what to do here exactly. I've stopped using string keys. Is there anything further?

Conversation continued at #8354 (comment)

src/components/structures/RoomStatusBar.tsx

…msc2716-marker-events

MadLittleMods · 2022-04-19T01:51:43Z

src/components/structures/RoomStatusBar.tsx

+        this.setState({
+            timelineNeedsRefresh: false,
+        });
+    };


I think the behavior of all of this would be best served by an end-to-end test. It looks like we just added Cypress recently but it's missing all of the utility necessary to complete something like this.

Needs a command to create a user behind the scenes and visit Element as the user already signed in

Needs commands to setup rooms and send events

Needs a way to enable experimental_features in the homeserver.yaml template

Needs a way to add an application service registration in the Synapse instance. The MSC2716 /batch_send endpoint is only accessible from a AS token. No extra AS server needed, just the AS token configured.

Needs a way to interact with the homserver directly from the application service token to call /batch_send (probably via matrix-js-sdk)

I can't find an overall issue describing the need/want to use Cypress so it's unclear how much we want to move forward with it. I've had many troubles using Cypress with Gitter.

It seems like we have other e2e tests using Puppeteer but I'm guessing we want to move all of these to Cypress. Better place I should be adding some e2e tests or approaching this?

Conversation continued at #8354 (comment)

See #8303 (comment)

MadLittleMods · 2022-04-19T04:32:11Z

Closing in favor of #8354 which has the same branch name as matrix-org/matrix-js-sdk#2299

See matrix-org/matrix-react-sdk#8303 (comment)

MadLittleMods · 2022-04-19T05:34:46Z

src/components/structures/TimelinePanel.tsx

        if (timelineSet !== this.props.timelineSet) return;

-        if (this.messagePanel.current && this.messagePanel.current.isAtBottom()) {
+        if (this.canResetTimeline()) {


This seems like a missed refactor. canResetTimeline by name seems aplicable here and the logic appears pretty equivalent.

matrix-react-sdk/src/components/structures/RoomView.tsx

Lines 1004 to 1009 in 27118a9

public canResetTimeline = () => {

if (!this.messagePanel) {

return true;

}

return this.messagePanel.canResetTimeline();

};

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

Line 638 in 27118a9

public canResetTimeline = () => this.messagePanel?.current.isAtBottom();

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

66ea757

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

MadLittleMods added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Apr 13, 2022

MadLittleMods requested a review from a team as a code owner April 13, 2022 01:08

MadLittleMods marked this pull request as draft April 13, 2022 01:08

MadLittleMods mentioned this pull request Apr 13, 2022

Draft: Refresh room timeline when we see a MSC2716 marker event matrix-org/matrix-js-sdk#2282

Closed

8 tasks

MadLittleMods commented Apr 13, 2022

View reviewed changes

turt2live reviewed Apr 13, 2022

View reviewed changes

MadLittleMods added 4 commits April 12, 2022 21:06

Back to false after getting screenshot

6dba3df

Fix lints

12b0115

Fix room not showing refresh timeline banner if not switched to the r…

eb5e899

…oom when the marker was sent

Fix type lints

a50e011

MadLittleMods mentioned this pull request Apr 14, 2022

Timeline needs to refresh when we see a MSC2716 marker event matrix-org/matrix-js-sdk#2299

Merged

3 tasks

Remove parenthesis change

ed910bb

MadLittleMods changed the title ~~Draft: Ask to refresh timeline when historical messages are imported (MSC2716)~~ Ask to refresh timeline when historical messages are imported (MSC2716) Apr 14, 2022

MadLittleMods marked this pull request as ready for review April 14, 2022 06:55

MadLittleMods requested a review from a team April 14, 2022 06:55

t3chguy reviewed Apr 14, 2022

View reviewed changes

src/components/structures/RoomStatusBar.tsx Outdated Show resolved Hide resolved

MadLittleMods added 3 commits April 16, 2022 02:17

Raw refreshLiveTimeline usage that seems to work

8d61226

Clean up raw commit

d8f94ed

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

c43d309

…msc2716-marker-events

MadLittleMods commented Apr 19, 2022

View reviewed changes

MadLittleMods added 2 commits April 18, 2022 23:20

Clean up event usage

623960b

See #8303 (comment)

Remove unreadable image descriptions

d9001ce

See #8303 (comment)

MadLittleMods mentioned this pull request Apr 19, 2022

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

Closed

3 tasks

MadLittleMods closed this Apr 19, 2022

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

Remove event remitters

52ce184

See matrix-org/matrix-react-sdk#8303 (comment)

MadLittleMods commented Apr 19, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

MadLittleMods commented Apr 13, 2022 •

edited by github-actions bot

Loading

MadLittleMods Apr 13, 2022 •

edited

Loading

MadLittleMods Apr 14, 2022

turt2live left a comment

codecov bot commented Apr 13, 2022 •

edited

Loading

t3chguy Apr 14, 2022

t3chguy Apr 14, 2022

MadLittleMods Apr 14, 2022

t3chguy Apr 14, 2022

MadLittleMods Apr 19, 2022

MadLittleMods Apr 19, 2022

MadLittleMods Apr 19, 2022 •

edited

Loading

MadLittleMods Apr 19, 2022

MadLittleMods commented Apr 19, 2022

MadLittleMods Apr 19, 2022

	public canResetTimeline = () => {
	if (!this.messagePanel) {
	return true;
	}
	return this.messagePanel.canResetTimeline();
	};

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

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

Conversation

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

Beware: edge case

Dev notes

Todo

✨ Features

MadLittleMods Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turt2live left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 13, 2022 • edited Loading

Codecov Report

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

MadLittleMods Apr 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MadLittleMods commented Apr 19, 2022

Choose a reason for hiding this comment

MadLittleMods commented Apr 13, 2022 •

edited by github-actions bot

Loading

MadLittleMods Apr 13, 2022 •

edited

Loading

codecov bot commented Apr 13, 2022 •

edited

Loading

MadLittleMods Apr 19, 2022 •

edited

Loading