feat(ui): Implement `Notification` API #2023

bnjbvr · 2023-06-06T17:25:03Z

This is the first PR for splitting the sync loop into two. This offers a new high-level API, NotificationApi, that makes use of a separate SlidingSync instance which sole role is to listen to to-device events and e2ee; it's pre-configured to do so. That means we're not force-enabling e2ee and to-device by default for every sliding sync instance, and as such we won't either generate Olm requests to the home server in general.

In the future, this new high-level API will hide some low-level dirty details so that its can be instantiated in multiple processes at the same time (lock across process, invalidate and refill crypto caches, etc.).

An embedder who would want to make use of this would need the following:

a main sliding sync instance, without e2ee and to-device. Using the matrix_sdk_ui::RoomList would be the best bet, at this time.
an instance of this matrix_sdk_ui::NotificationApi, with a different identifier.

Note that this is not ready to be used in an external process; or it will cause the same kind of issues that we're seeing as of today: invalid crypto caches resulting in UTD, etc.

This PR can be looked at, commit per commit, at the time of writing this.

Fixes #1961.

codecov · 2023-06-06T17:56:15Z

Codecov Report

Patch coverage: 88.57% and project coverage change: +0.05 🎉

Comparison is base (76ed351) 75.91% compared to head (62e39ac) 75.96%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2023      +/-   ##
==========================================
+ Coverage   75.91%   75.96%   +0.05%     
==========================================
  Files         152      153       +1     
  Lines       16725    16749      +24     
==========================================
+ Hits        12696    12723      +27     
+ Misses       4029     4026       -3

Impacted Files	Coverage Δ
crates/matrix-sdk-ui/src/notifications/mod.rs	`84.21% <84.21%> (ø)`
crates/matrix-sdk/src/sliding_sync/mod.rs	`92.46% <90.00%> (+0.66%)`	⬆️
crates/matrix-sdk-ui/src/room_list/mod.rs	`89.47% <100.00%> (+0.28%)`	⬆️
crates/matrix-sdk/src/sliding_sync/builder.rs	`66.11% <100.00%> (+2.18%)`	⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

crates/matrix-sdk-ui/src/notifications/mod.rs

Hywan

The code looks generally good. Some minors stuff to update. We need to test this for real to see if it behaves as expected.

bindings/matrix-sdk-crypto-nodejs/src/machine.rs

crates/matrix-sdk-ui/Cargo.toml

crates/matrix-sdk-ui/src/notifications/mod.rs

crates/matrix-sdk-ui/tests/integration/main.rs

crates/matrix-sdk-ui/tests/integration/notification.rs

crates/matrix-sdk-ui/tests/integration/room_list.rs

crates/matrix-sdk/src/sliding_sync/mod.rs

…evice events Signed-off-by: Benjamin Bouvier <public@benj.me>

Signed-off-by: Benjamin Bouvier <public@benj.me>

…nsion Signed-off-by: Benjamin Bouvier <public@benj.me>

… API testing Signed-off-by: Benjamin Bouvier <public@benj.me>

Signed-off-by: Benjamin Bouvier <public@benj.me>

Too cute? :) Signed-off-by: Benjamin Bouvier <public@benj.me>

…loop terminates Signed-off-by: Benjamin Bouvier <public@benj.me>

…n sync loop This makes sure that we `sync()` the notification sync loop only once. Signed-off-by: Benjamin Bouvier <public@benj.me>

Signed-off-by: Benjamin Bouvier <public@benj.me>

bnjbvr · 2023-06-13T14:23:51Z

The macro used in the RoomList and Notification integration testing now supports sticky parameters \o/

The mocked server will parrot the txn_id provided in the request, so that sticky parameters are validated.
Before checking the response matches what we expect, we strip the txn_id from the response, as it's been auto-generated it's not possible to include it in the expected side of the assertion.

Signed-off-by: Benjamin Bouvier <public@benj.me>

Hywan

I'm all good, I just want to check again the RoomList modification, esp. on the test suite :-).

crates/matrix-sdk-ui/src/notifications/mod.rs

Hywan · 2023-06-16T12:28:24Z

crates/matrix-sdk-ui/src/notifications/mod.rs

+///
+/// See the module's documentation for more details.
+#[derive(Clone)]
+pub struct NotificationSync {


What about E2EESync or EncryptionSync? Because as far as I understand it, it has very little to do with notifications so far, except the lock maybe?

Nice, I've also discussed with other people and thought about EncryptionSync, so let's go with that!

And if that's alright, I'd rather do it in the next PR, to not mess up too much with my rebase 😅

crates/matrix-sdk-ui/src/notifications/mod.rs

crates/matrix-sdk-ui/tests/integration/notification.rs

Hywan · 2023-06-16T12:39:13Z

crates/matrix-sdk-ui/tests/integration/room_list.rs

-                "e2ee": {
-                    "enabled": true,
-                },
-                "to_device": {
-                    "enabled": true,
-                },


That's wrong, we should have those :-).

Noooo! ... or maybe, you're right, at least on Android :-)

On iOS, the main app will use both the RoomList and EncryptionSync, so the RoomList on iOS mustn't have these two extensions. Maybe we could make it a parameter in the room list ctor? Or have Android spawn both too, at least for symmetry with iOS? (and so it gets some of other benefits: e2ee not being blocked by processing of lists on the ss proxy, etc.)

Hmm, let's keep this as is for now :-). Sorry for the confusion!

Hywan · 2023-06-16T12:39:33Z

crates/matrix-sdk-ui/tests/integration/room_list.rs

            "lists": {
                ALL_ROOMS: {
                    "ranges": [
                        [0, 49],
                    ],
-                    "timeline_limit": 1,


Why did you remove this one?

I've added support for sticky parameters in the test macro, and this one is sticky, so it's not reemitted in the next request, since it's the same :)

Signed-off-by: Benjamin Bouvier <public@benj.me>

Hywan · 2023-06-19T07:47:02Z

crates/matrix-sdk-ui/src/notifications/mod.rs

+///
+/// See the module's documentation for more details.
+#[derive(Clone)]
+pub struct NotificationSync {


Hywan · 2023-06-19T07:48:20Z

crates/matrix-sdk-ui/tests/integration/room_list.rs

-                "e2ee": {
-                    "enabled": true,
-                },
-                "to_device": {
-                    "enabled": true,
-                },


Hmm, let's keep this as is for now :-). Sorry for the confusion!

Signed-off-by: Benjamin Bouvier <public@benj.me>

bnjbvr requested a review from a team as a code owner June 6, 2023 17:25

bnjbvr requested review from poljar and Hywan and removed request for a team June 6, 2023 17:25

jplatte reviewed Jun 7, 2023

View reviewed changes

crates/matrix-sdk-ui/src/notifications/mod.rs Outdated Show resolved Hide resolved

Hywan requested changes Jun 7, 2023

View reviewed changes

Hywan changed the title ~~sliding sync: implement a NotificationApi~~ feat(ui): Implement Notification Jun 7, 2023

bnjbvr force-pushed the split-sync-loop-take-3 branch 3 times, most recently from 1dbddde to 9decb06 Compare June 8, 2023 10:32

bnjbvr changed the title ~~feat(ui): Implement Notification~~ feat(ui): Implement Notification API Jun 8, 2023

bnjbvr added 13 commits June 12, 2023 14:21

feat: add a NotificationApi that takes care of sync'ing e2ee and to-d…

dc706ae

…evice events Signed-off-by: Benjamin Bouvier <public@benj.me>

adjust test expectations for respecting consent

bb23f20

Signed-off-by: Benjamin Bouvier <public@benj.me>

feat: send the outgoing e2ee requests iff we've enabled the e2ee exte…

ebce783

…nsion Signed-off-by: Benjamin Bouvier <public@benj.me>

test: refactor so notification API can reuse the infra from room list…

234a770

… API testing Signed-off-by: Benjamin Bouvier <public@benj.me>

chore: cargo fmt

45159db

Signed-off-by: Benjamin Bouvier <public@benj.me>

feat: enable the conn_id field in the sliding sync request

7419db1

Signed-off-by: Benjamin Bouvier <public@benj.me>

chore: fix typo :)

61347c0

Signed-off-by: Benjamin Bouvier <public@benj.me>

feat: expose the NotificationApi in the FFI

84706e2

Signed-off-by: Benjamin Bouvier <public@benj.me>

chore: address review comments

9c45a99

Signed-off-by: Benjamin Bouvier <public@benj.me>

test: generalize the macro so it can handle subset and strict JSON diffs

7b140a4

Too cute? :) Signed-off-by: Benjamin Bouvier <public@benj.me>

feat: add a listener that is notified whenever the notification sync …

8c0c632

…loop terminates Signed-off-by: Benjamin Bouvier <public@benj.me>

ffi: expose a simpler object that holds the handle to the notificatio…

7f4e275

…n sync loop This makes sure that we `sync()` the notification sync loop only once. Signed-off-by: Benjamin Bouvier <public@benj.me>

chore: use previous setters when creating the notification sliding sync

b64781b

Signed-off-by: Benjamin Bouvier <public@benj.me>

bnjbvr force-pushed the split-sync-loop-take-3 branch from 5131501 to b64781b Compare June 12, 2023 12:42

bnjbvr added 4 commits June 12, 2023 17:43

doh: don't swallow sliding sync error silently

2b4545c

Signed-off-by: Benjamin Bouvier <public@benj.me>

doh: infer the sliding-sync proxy URL, if not manually configured

50d29e0

Signed-off-by: Benjamin Bouvier <public@benj.me>

Merge branch 'main' into split-sync-loop-take-3

1582788

chore: update test expectations

2bdc7ab

Signed-off-by: Benjamin Bouvier <public@benj.me>

doh: fix merge issues

06bb05a

Signed-off-by: Benjamin Bouvier <public@benj.me>

bnjbvr requested a review from Hywan June 13, 2023 14:35

Merge branch 'main' into split-sync-loop-take-3

5e5e1f4

Hywan requested changes Jun 16, 2023

View reviewed changes

bnjbvr added 2 commits June 16, 2023 16:43

Merge branch 'main' into split-sync-loop-take-3

bd31385

review comments!

75a5268

Signed-off-by: Benjamin Bouvier <public@benj.me>

bnjbvr requested a review from Hywan June 16, 2023 14:49

update tests

b3fc29d

Signed-off-by: Benjamin Bouvier <public@benj.me>

Hywan approved these changes Jun 19, 2023

View reviewed changes

last-minute cleanups

62e39ac

Signed-off-by: Benjamin Bouvier <public@benj.me>

bnjbvr enabled auto-merge (squash) June 19, 2023 08:38

bnjbvr merged commit 4d3ca15 into matrix-org:main Jun 19, 2023
47 checks passed

bnjbvr deleted the split-sync-loop-take-3 branch June 19, 2023 08:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ui): Implement `Notification` API #2023

feat(ui): Implement `Notification` API #2023

bnjbvr commented Jun 6, 2023 •

edited

codecov bot commented Jun 6, 2023 •

edited

Hywan left a comment

bnjbvr commented Jun 13, 2023

Hywan left a comment

Hywan Jun 16, 2023

bnjbvr Jun 16, 2023

bnjbvr Jun 16, 2023

Hywan Jun 19, 2023

Hywan Jun 16, 2023

bnjbvr Jun 16, 2023

Hywan Jun 19, 2023

Hywan Jun 16, 2023

bnjbvr Jun 16, 2023

Hywan Jun 19, 2023

Hywan Jun 19, 2023

feat(ui): Implement Notification API #2023

feat(ui): Implement Notification API #2023

Conversation

bnjbvr commented Jun 6, 2023 • edited

codecov bot commented Jun 6, 2023 • edited

Codecov Report

Hywan left a comment

Choose a reason for hiding this comment

bnjbvr commented Jun 13, 2023

Hywan left a 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

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

Choose a reason for hiding this comment

feat(ui): Implement `Notification` API #2023

feat(ui): Implement `Notification` API #2023

bnjbvr commented Jun 6, 2023 •

edited

codecov bot commented Jun 6, 2023 •

edited