Skip to content
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

Load Thread List with server-side assistance (MSC3856) #2602

Merged
merged 18 commits into from
Oct 5, 2022

Conversation

justjanne
Copy link
Contributor

@justjanne justjanne commented Aug 18, 2022

Type: Enhancement
Related: element-hq/element-web#21877
Related: element-hq/element-web#21911

  • Load thread list with MSC3856
  • Working fallback for older servers
  • Update thread in thread list when new messages are added
  • Update thread in thread list when old messages are deleted

Here's what your changelog entry will look like:

✨ Features

@justjanne justjanne force-pushed the justjanne/feat/thread-list-api-proof-of-concept branch from aec5b7e to 4dfe884 Compare August 18, 2022 12:46
@justjanne justjanne force-pushed the justjanne/feat/thread-list-api-proof-of-concept branch 2 times, most recently from c1fd56a to c5aff2d Compare September 14, 2022 14:46
@justjanne justjanne force-pushed the justjanne/feat/thread-list-api-proof-of-concept branch 2 times, most recently from a3be071 to ba48a6f Compare September 23, 2022 12:34
@justjanne justjanne changed the title proof of concept for thread list api implementation Load Thread List with server-side assistance (MSC3856) Sep 23, 2022
@justjanne justjanne force-pushed the justjanne/feat/thread-list-api-proof-of-concept branch 2 times, most recently from d5e339b to 662fe0b Compare September 26, 2022 11:34
@justjanne justjanne marked this pull request as ready for review September 27, 2022 13:31
@justjanne justjanne requested a review from a team as a code owner September 27, 2022 13:31
Copy link
Member

@robintown robintown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few questions, also as the CI notes this could use just a little bit more coverage

src/models/room.ts Outdated Show resolved Hide resolved
src/models/room.ts Outdated Show resolved Hide resolved
src/models/room.ts Outdated Show resolved Hide resolved
src/models/room.ts Outdated Show resolved Hide resolved
src/models/room.ts Outdated Show resolved Hide resolved
src/client.ts Outdated Show resolved Hide resolved
spec/integ/matrix-client-event-timeline.spec.ts Outdated Show resolved Hide resolved
spec/integ/matrix-client-event-timeline.spec.ts Outdated Show resolved Hide resolved
spec/integ/matrix-client-event-timeline.spec.ts Outdated Show resolved Hide resolved
spec/integ/matrix-client-event-timeline.spec.ts Outdated Show resolved Hide resolved
Copy link
Member

@robintown robintown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now as long as CI becomes happy

@justjanne justjanne force-pushed the justjanne/feat/thread-list-api-proof-of-concept branch from e3aafbc to fbb39af Compare October 4, 2022 12:23
@justjanne justjanne merged commit 3a3dcfb into develop Oct 5, 2022
@justjanne justjanne deleted the justjanne/feat/thread-list-api-proof-of-concept branch October 5, 2022 21:10
su-ex added a commit to SchildiChat/matrix-js-sdk that referenced this pull request Oct 29, 2022
* Changes the `uploadContent` API, kills off `request` and `browser-request` in favour of `fetch`, removed callback support on a lot of the methods, adds a lot of tests. ([\matrix-org#2719](matrix-org#2719)). Fixes matrix-org#2415 and matrix-org#801.
* Remove deprecated `m.room.aliases` references ([\matrix-org#2759](matrix-org#2759)). Fixes element-hq/element-web#12680.
* Remove node-specific crypto bits, use Node 16's WebCrypto ([\matrix-org#2762](matrix-org#2762)). Fixes matrix-org#2760.
* Export types for MatrixEvent and Room emitted events, and make event handler map types stricter ([\matrix-org#2750](matrix-org#2750)). Contributed by @stas-demydiuk.
* Use even more stable calls to `/room_keys` ([\matrix-org#2746](matrix-org#2746)).
* Upgrade to Olm 3.2.13 which has been repackaged to support Node 18 ([\matrix-org#2744](matrix-org#2744)).
* Fix `power_level_content_override` type ([\matrix-org#2741](matrix-org#2741)).
* Add custom notification handling for MSC3401 call events  ([\matrix-org#2720](matrix-org#2720)).
* Add support for unread thread notifications ([\matrix-org#2726](matrix-org#2726)).
* Load Thread List with server-side assistance (MSC3856) ([\matrix-org#2602](matrix-org#2602)).
* Use stable calls to `/room_keys` ([\matrix-org#2729](matrix-org#2729)). Fixes element-hq/element-web#22839.
* Fix POST data not being passed for registerWithIdentityServer ([\matrix-org#2769](matrix-org#2769)). Fixes matrix-org/element-web-rageshakes#16206.
* Fix IdentityPrefix.V2 containing spurious `/api` ([\matrix-org#2761](matrix-org#2761)). Fixes element-hq/element-web#23505.
* Always send back an httpStatus property if one is known ([\matrix-org#2753](matrix-org#2753)).
* Check for AbortError, not any generic connection error, to avoid tightlooping ([\matrix-org#2752](matrix-org#2752)).
* Correct the dir parameter of MSC3715 ([\matrix-org#2745](matrix-org#2745)). Contributed by @dhenneke.
* Fix sync init when thread unread notif is not supported ([\matrix-org#2739](matrix-org#2739)). Fixes element-hq/element-web#23435.
* Use the correct sender key when checking shared secret ([\matrix-org#2730](matrix-org#2730)). Fixes element-hq/element-web#23374.
@@ -711,6 +752,63 @@ describe("MatrixClient event timelines", function() {
});

describe("getLatestTimeline", function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also add some tests for the thread scenarios added to getLatestTimeline(...) in this PR.

It was originally only intended for the main live timeline (room.getUnfilteredTimelineSet(...)) and our only usage in the SDK from room.refreshLiveTimeline(...) is still for that purpose

@@ -5438,27 +5444,36 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
* @return {Promise} Resolves:
* {@link module:models/event-timeline~EventTimeline} timeline with the latest events in the room
*/
public async getLatestTimeline(timelineSet: EventTimelineSet): Promise<EventTimeline> {
public async getLatestTimeline(timelineSet: EventTimelineSet): Promise<Optional<EventTimeline>> {
Copy link
Contributor

@MadLittleMods MadLittleMods Nov 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is expected to always return an EventTimeline and was introduced during a time where getEventTimeline(...) always returned a timeline as well, see #2299. I see that this PR is just updating the signature to match what getEventTimeline(...) can do to us though.

This will break the assumptions in room.refreshLiveTimeline()

For reference, we're also thinking about how to make getEventTimeline(...) play nicely here for another bug scenario in #2521 (comment). So there probably isn't any action here besides sparing a thought if you can think of a good solution.

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

Successfully merging this pull request may close these issues.

3 participants