MSC3664: Pushrules for relations

[deepbluev7]

trafficstars

Rendered

Partial implementation was done by Beeper, but without the appropriate namespacing and it is missing the filtering by rel_type: https://gitlab.com/beeper/synapse/-/commit/44a1728b6b021f97900c89e0c00f7d1a23ce0d43

Implementation: https://github.com/matrix-org/synapse/pull/11804 (Note: I have no idea, what I am doing)

Signed-off-by: Nicolas Werner [email protected]

Preview: https://pr3664--matrix-org-previews.netlify.app

FCP Proposal

[erikjohnston]

Seems like a bunch of people are vaguely happy with this, so...

@mscbot fcp merge

[mscbot]

This FCP proposal has been cancelled by https://github.com/matrix-org/matrix-spec-proposals/pull/3664#issuecomment-1174029278.

Team member @mscbot has proposed to merge this. The next step is review by the rest of the tagged people:

• [ ] @dbkr
• [ ] @uhoreg
• [ ] @turt2live
• [ ] @ara4n
• [ ] @anoadragon453
• [ ] @richvdh
• [x] @erikjohnston
• [ ] @KitsuneRal

Concerns:

• lacks tested implementation
• backwards-compatibility problem not mentioned
• implicit conversion of replies
• Need to consider the threads team's opinions (whatever they are)

Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for information about what commands tagged team members can give me.

[richvdh]

@mscbot concern lacks tested implementation

while https://github.com/matrix-org/synapse/pull/11804 is a plausible proof of concept, I'm a bit worried that:

• it hasn't been merged to synapse mainline, and appears to require some work before it can (it declares itself to be "inefficient"?)
• generally, I'm unsure how much testing it has received. We have a bit of a history of making changes to push rules which are "obviously correct", but then fail to survive engagement with ~~the enemy~~ users. Typically the wheels fall off once attempts are made to enable the new push rules on real user accounts.

So, I'd really be a lot more comfortable with (a) a merged PR, and (b) some evidence of wider testing.

[richvdh]

@mscbot concern backwards-compatibility problem not mentioned

https://github.com/matrix-org/matrix-spec-proposals/pull/3664#discussion_r789520216 discusses a problem for new clients (which expect the push rule to exist) talking to old servers. Please can something be included in the MSC body about it.

[richvdh]

@mscbot concern implicit conversion of replies

https://github.com/matrix-org/matrix-spec-proposals/pull/3664#discussion_r880803665

[turt2live]

before I dive into this, I'd like to see what Threads has in mind for notifications. Implicitly, this means an approving review from people working on that feature.

@mscbot concern Need to consider the threads team's opinions (whatever they are)

[turt2live]

This does not appear to be ready for FCP: Implementation was declared insufficient, which alone is enough to back this out of its current FCP state.

@mscbot fcp cancel

[deepbluev7]

Well, the implementation is blocked on #3825 since that would break notifications for threads instead of solving them.

[turt2live]

https://github.com/matrix-org/matrix-spec-proposals/pull/3825 appears to be stuck and https://github.com/matrix-org/matrix-spec-proposals/pull/2781 depends on this MSC - what are the next steps to make this go forward?

[deepbluev7]

Well, you can kinda ignore #3825 when implementing this MSC. It just makes the implementation more bothersome, but my client SDK already transforms the events using special rules for threads. Since we can get the SCT to accept #3825, we can just move ahead without it and add a "Note" box in the spec to explain how to do it properly.

[bradtgmurray]

Just wanted to jump in and thank @deepbluev7 for continuing to push this forward. We're using this functionality to enable notifications when someone reacts to one of the user's messages but ignore reactions to other user's messages, and I'm excited to flip from our custom implementation to the one that's been included in Synapse 1.70.

[bradtgmurray]

We're now running this in production, here's an example base push rule for our users where users get notifications for reactions, but only to their messages, and only in rooms that are "small" (less than 20 members).

https://github.com/beeper/synapse/blob/ad6b10b70df5befa0d39726bd3481a6fbbdc41ce/rust/src/push/base_rules.rs#L420-L446