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

Old reactions should be hidden when a message is edited. #10136

Closed
jryans opened this issue Jun 21, 2019 · 10 comments
Closed

Old reactions should be hidden when a message is edited. #10136

jryans opened this issue Jun 21, 2019 · 10 comments

Comments

@jryans
Copy link
Collaborator

jryans commented Jun 21, 2019

No description provided.

@bwindels
Copy link
Contributor

bwindels commented Jul 2, 2019

I imagine eventually we want the server to do this ... ? Although we don't have a precedent really for the server to insert more events into the timeline upon somebody sending an event... This wouldn't work for e2e... or would it? redacts is in unsigned ...

Doing it client-side seems suboptimal as well ... you could not be aware of all edits ... send could fail ... you could be using a client that doesn't do this.

Does it make sense to spend time to do this client-side in riot in the short term, given the above?

@lampholder
Copy link
Member

I don't know that we necessarily want to be throwing reactions away ever, otherwise somebody can say somethign obnoxious, receive a lot of negative feedback, then just edit their message (to something equally obnoxious if they like) and wipe the slate clean.

How tractable is it to have reactions key off the edit rather than the original message? (Understanding this is probably an invasive change to spec etc.)

@jryans
Copy link
Collaborator Author

jryans commented Jul 2, 2019

I don't know that we necessarily want to be throwing reactions away ever, otherwise somebody can say somethign obnoxious, receive a lot of negative feedback, then just edit their message (to something equally obnoxious if they like) and wipe the slate clean.

The idea of clearing reactions after an edit was proposed for the opposite scenario, I believe: to create social pressure for edits to die down once the message looks good, so that you can then collect reactions and the reactions senders can be assured that the message hasn't changed since they interacted with it.

@jryans
Copy link
Collaborator Author

jryans commented Jul 2, 2019

How tractable is it to have reactions key off the edit rather than the original message? (Understanding this is probably an invasive change to spec etc.)

It does sound complex, to be honest:

  • Clients would have to now consider not just the original message but also edits as possible reaction targets
  • Servers would need to work out how to properly interleave all these things together bundling
  • Spec would need to describe how paginating and other APIs are meant to work in this fashion

@lampholder
Copy link
Member

Okay - I think there might be utility in exploring this more further down the road, but for now let's just have edits dispose of associated reactions.

@lampholder lampholder added phase:3 and removed phase:2 labels Jul 3, 2019
@lampholder lampholder changed the title Reactions should be cleared when editing a message Old reactions should be hidden when a message is edited. Jul 3, 2019
@lampholder
Copy link
Member

We've had a slightly confused conversation about this; most of the confusion stemmed from whether 'cleared' meant delete from the database or not. We (@jryans and I) are now in agreement that the data would not be deleted - it would only be hidden in the UI.

If were were to proceed with this hiding-outdated-reactions feature, we would need to think about:

  • sending reactions to target the edit, not the message
  • clients' checking edits for reactions to display
  • servers have to look for reactions on edits when bundling

This sounds like a reasonable chunk of work, that is desirable to make the edits/reactions features work together without exposing edge cases for potential abuse. But it is probably not necessary for phase:2

@lampholder lampholder added phase:3 and removed phase:2 labels Jul 3, 2019
@bwindels
Copy link
Contributor

If we do this thing, it should probably be added to the spec, so that all clients that implement reactions react to that last edit. Otherwise you'd react in some client, and it wouldn't show up in others.

@bwindels
Copy link
Contributor

Deploying this change should be (somewhat) aligned across mobile and web, as they would stop showing each other reactions once a message is edited.

Deploying this without the required changes in server-side bundling probably also doesn't make sense, as it will just break the server-side aggregation we already use.

You could also argue that the server should reject reactions on events that are not the last edit, to avoid clients posting reactions that won't be shown elsewhere.

So marking this as blocked until there is backend bandwidth to work on this.

@Biep
Copy link

Biep commented Sep 5, 2019

My proposal would be greying out, or otherwise marking, any reactions prior to the last redaction.

@SimonBrandner SimonBrandner added X-Needs-Design X-Needs-Product More input needed from the Product team labels Feb 10, 2022
@turt2live
Copy link
Member

I feel we've made the implicit product/design decision on this having not actioned it. If people feel strongly otherwise, please open a new issue with fresh information for reconsideration.

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

No branches or pull requests

6 participants