-
Notifications
You must be signed in to change notification settings - Fork 385
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
MSC4142: Remove unintentional intentional mentions in replies #4142
Open
tulir
wants to merge
2
commits into
main
Choose a base branch
from
tulir/fix-reply-intentional-mentions
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
# MSC4142: Remove unintentional intentional mentions in replies | ||
|
||
Currently, the reply spec has a section called [Mentioning the replied to user](https://spec.matrix.org/v1.10/client-server-api/#mentioning-the-replied-to-user) | ||
which says | ||
|
||
> In order to notify users of the reply, it may be desirable to include the | ||
> sender of the replied to event and any users mentioned in that event. See | ||
> [user and room mentions](https://spec.matrix.org/v1.10/client-server-api/#user-and-room-mentions) | ||
> for additional information. | ||
|
||
The "*any users mentioned in that event*" part is particularly problematic, as | ||
it effectively means all mentions will be propagated forever through a reply | ||
chain, causing lots of unintentional pings. | ||
clokep marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
The propagation was originally added to preserve the old reply fallback mention | ||
behavior where explicit mentions in the replied-to message were be copied to | ||
the reply fallback and therefore caused pings. However, the current spec copies | ||
far more than just explicit pings from the replied-to message. Additionally, no | ||
other chat application that I know of propagates mentions like that. | ||
|
||
## Proposal | ||
The proposed fix is to stop propagating mentions entirely. The `m.mentions` | ||
object of replies should only contain explicit mentions in the new message, | ||
plus the sender of the replied-to message. The mentions in the replied-to | ||
message are ignored. | ||
|
||
Clients are still free to add other mentions to the list as they see fit. For | ||
example, a client could offer a button to mention all users in a reply chain. | ||
This proposal simply changes the default behavior recommended in the spec. | ||
|
||
## Potential issues | ||
Users who have already got used to the new behavior may be surprised when they | ||
don't get mentioned by reply chains. | ||
|
||
## Alternatives | ||
### Split `m.mentions` | ||
To preserve the old reply fallback behavior, `m.mentions` could be split into | ||
"explicit" and "implicit", so that replies copy explicit mentions into the | ||
implicit list. Future replies would then only copy new explicit pings and | ||
wouldn't cause an infinite chain. | ||
|
||
Since other chat applications don't copy pings at all, having a weird feature | ||
like that doesn't seem worth the additional complexity. | ||
|
||
## Security considerations | ||
This proposal doesn't touch anything security-sensitive. | ||
|
||
## Unstable prefix | ||
Not applicable, this proposal only changes how the existing `m.mentions` object | ||
is filled for replies. | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation requirements:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not entirely sure what that means 🤔 The PR opening comment has an example of the currently specced behavior being undesired, and I think it's relatively obvious that mentioning the user you're replying to is desired
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notifications have always been a pretty controversial area to be within. Where some folks consider it a bug, others don't.
That requirement is mostly to say it needs opinion from Product-centered folks.