MSC2677: Annotations and reactions

[uhoreg]

Shepherd: @richvdh

Rendered

Replaces #1849 along with #2674, #2675, and #2676

FCP tickyboxes

[uhoreg]

Transfer comments from 1849

[chc4]

Is this the Matrix spec proposal that is currently implemented by Element (and Synapse?) for their reactions? Did they actually implement it already without reactions being standardized, or is there some other message type I'm missing?

[tulir]

Yes. Proposals have to be proven to work with an implementation before they can be accepted, otherwise we'd end up with lots of fancy specs with no idea if they're going to work in the real world.

This proposal is a bit old (the original version, #1849, is from early 2019), which is why the event type is m.reaction instead of using an unstable prefix like org.matrix.msc1849.reaction. PoC implementations of newer proposals will use unstable prefixes as per #2324.

[chc4]

Huh, ok. I understand the reasoning, but having the reference client and reference server implement non-standardized parts of the Matrix spec is a pretty bad look.

[Cadair]

That's why #2324 was agreed upon so there is a clear line between the spec and implementations being ahead of the released spec. All projects like this grow organically, sometimes you just need to learn from your missteps.

[ShadowJonathan]

was brought up again in a matrix room, how is it that this MSC isn't FCPd yet? what things are remaining before this can become part of the normal spec?

[erkinalp]

Reactions of reactions and arbitrary text reactions

[AndrewRyanChama]

commented with concerns about the ability to display the correct reactions when seeking to an older message

[AndrewRyanChama]

In current implementations, if a message is deeplinked or otherwise seeked to in history, then reactions made to that message long afterwards will not be displayed.

I'm worried about possible parity issues with competing messaging apps, for example it's common for a pinned message or something to gather a gorillion or more reactions over a long time. This MSC should provide a way so that particularly important messages in the past that are repeatedly referenced can still accurately display their reactions.

With the current api it might be possible to call /relations for a single message, but it's not practical to call this on all messages when paginating, because it will cause too many api calls, and there is no way to know which messages have reactions on them.

Possibly aggregation isn't the right solution here, but we need a way such that clients have a way to display the correct reactions in these cases.

[KitsuneRal]

My take on it is that this is a problem specifically when paginating from a deeplinked message back or forth. Paginating back from the sync edge of the timeline is going to rake all reactions even before the related events; and I would probably be fine with explicitly calling /relations on pinned messages because normally there are not many of them, anyway. From the API standpoint that leaves /context and /search; as long as encrypting the fact of relationship is not on the cards, I'd probably rather explore getting reactions in the same way we get state events in those endpoints now.

[jplatte]

I think a good trade-off here could be to aggregate reactions into a plain 'number of reactions' server-side. Then clients will know whether they have seen all reactions and can use /relations to fetch them, if they care.

I'd also be strongly in favor of then encrypting the "key" of the reaction, the same way the new content is encrypted for edits but the relation to the event being edited isn't, but maybe that should be its own thread.

[Alcaro]

Doesn't that give weird results if someone removes a reaction? "20 reactions now, 20 reactions last time" proves nothing - Alice could've removed her thumb up and replaced with thumb down. Though "0 reactions now", which is true for 99% of messages, is usable information.

If you mean aggregating into "13 thumbs up, 7 thumbs down", that would be useful. Though if one of them is subsequently redacted, client won't know which type to decrement. (I'm not even sure if client knows which message was affected - if I'm reading the spec correctly, redactions only contain the ID of the redacted event, not which message that event is attached to.)

"13 thumbs up, 7 thumbs down" is also insufficient for clients who care about who reacted - Alice could've retracted her thumb up, and Bob added one instead - but that's also rare enough to ignore. Clients can just fetch the relations list for messages it cares about, which is presumably only a small number.

[dkasak]

I think @jplatte has the right idea, but instead of making it the number of reactions, I think it should be number of relations. Then clients should fetch all relations whenever the event has relations (and they're interested in them). This gives us the opportunity to encrypt both the key and the rel_type, as described here. (I don't think it's reasonable to encrypt the event ID, so ignore that part.)

[jplatte]

Doesn't that give weird results if someone removes a reaction? "20 reactions now, 20 reactions last time" proves nothing

Wdym with "now" and "last time"? The client would only be seeing this one time when fetching the event, then if it cares about reactions, it will fetch all the reactions via /relations, and receive updates as additional (reaction / redaction) events.

[jplatte]

@dkasak I like it!

[Alcaro]

Wdym with "now" and "last time"?

I'm considering the case where the client sees the same message multiple times, for example fetching the pins twice, or a large segment of recent channel history, and doesn't want to waste bandwidth on refetching unchanged reaction counts.

But it sounds like that's not the usecase you're optimizing for, so I apologize for the noise.

[richvdh]

It's true that current implementations are "buggy" in the case of old messages with reactions the client hasn't seen, and the discussion of potential solutions is welcome.

However, I think I should reiterate the purpose of this MSC, as set out in its introduction:

... this MSC therefore seeks primarily to formalise the status quo. Although there is plenty of scope for improvement, we consider that better done in future MSCs, based on a shared understanding of the current implementation.

In other words, sorry, but we absolutely won't be redesigning reaction aggregations as part of this MSC. Trivial changes are within scope, and it turned out that ripping out the existing implementation was easy and made more sense than leaving it in, for the reasons discussed at #2677 (comment). However anything else is going to need implementing and testing on both servers and clients and is just going to end up delaying this MSC yet again.

In other other words, the options for this MSC are (a) keep the aggregation as it was before 749198f and matrix-org/synapse#15172 or (b) rip it out. There is no (c).

[gnunicorn]

Someone brought the question up the other day, whether the concepts (and even the implementation) of GraphQL isn't the better suited idea for the API (rather than plain old REST) now that we have relational data and clients, who want to pro-actively decide very granular which information they want. Just wanted to leave that here as a side-note ...

[mscbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[mscbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

[turt2live]

Spec PR: matrix-org/matrix-spec#1475

[turt2live]

Merged 🎉