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

Receipt for a redaction within a thread is sent to the main thread #26363

Closed
andybalaam opened this issue Oct 13, 2023 · 11 comments
Closed

Receipt for a redaction within a thread is sent to the main thread #26363

andybalaam opened this issue Oct 13, 2023 · 11 comments
Assignees
Labels
A-Notifications O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect

Comments

@andybalaam
Copy link
Contributor

andybalaam commented Oct 13, 2023

Part of #24392 (Stuck Notifications)

(Found while debugging "Reading an unread thread after a redaction of the latest message makes it read" inside cypress/e2e/read-receipts/redactions.spec.ts.)

When I open an unread thread where the last message was a redaction of a message within that thread, I see this request in the Network tab in my browser:

http://example.com/_matrix/client/v3/rooms/!roomid/receipt/m.read/%24eventtoredactid

{
  "thread_id": "main"
}

which is incorrect, because the redaction event happened within the thread, so we should be sending a body of:

{
    "thread_id": "$mythreadid"
}
@andybalaam andybalaam added T-Defect S-Minor Impairs non-critical functionality or suitable workarounds exist O-Occasional Affects or can be seen by some users regularly or most users rarely A-Notifications labels Oct 13, 2023
@andybalaam andybalaam self-assigned this Oct 13, 2023
@t3chguy
Copy link
Member

t3chguy commented Oct 13, 2023

If an event is redacted its relation is also removed so it loses its thread-ness

The content object must also be stripped of all keys, unless it is one of the following event types:

https://spec.matrix.org/v1.8/rooms/v11/#redactions

@andybalaam
Copy link
Contributor Author

OK, that explains why when I change this behaviour, Synapse complains that it doesn't think the reaction is within that thread.

So I think this means that when Element Web sees a redaction of a message within a thread, it should move that message out of the thread's timeline and place it in the main timeline. Then we would not consider that redacted message or its redaction to be in the thread, so we wouldn't try to send threaded receipts for it.

What do you think @t3chguy ?

If so, we still need to insert it into the main timeline, and I'm not at all sure where. I think we can only guess based on timestamp (again).

@t3chguy
Copy link
Member

t3chguy commented Oct 13, 2023

I think the spec should offer an explanation on how clients are expected to handle this so we don't build Synapse-specific behaviours which break on Conduit, Dendrite etc

@andybalaam
Copy link
Contributor Author

I think the spec should offer an explanation on how clients are expected to handle this so we don't build Synapse-specific behaviours which break on Conduit, Dendrite etc

Which part is ambiguous?

@t3chguy
Copy link
Member

t3chguy commented Oct 13, 2023

How to handle thread reply redactions more broadly, as things moving between timelines inconsistently seems bad. Equally events vanishing from timelines without a marker of where they used to be also seems quite bad

@andybalaam
Copy link
Contributor Author

Yes, in this scenario:

  • I receive messages: ThreadRoot and InThread1, which is a thread reply to ThreadRoot
  • I read InThread1, sending a threaded read receipt.
  • I receive a redaction of InThread1. This removes its relationship information, meaning it is not considered part of the thread.

Now the latest receipt for the thread refers to a message (InThread1) which is no longer part of the thread.

@t3chguy
Copy link
Member

t3chguy commented Oct 13, 2023

Ignoring receipts entirely, it means messages vanish from threads and appear in the main timeline. They look like redactions in the main timeline which is confusing and arguably incorrect.

@andybalaam
Copy link
Contributor Author

So would you propose to preserve relations when events are redacted?

I.e. update the redaction algorithm to say that m.relates_to within content is always kept?

@t3chguy
Copy link
Member

t3chguy commented Oct 13, 2023

It would require a new room version, I suggest raising this with the SCT

@andybalaam
Copy link
Contributor Author

So this is not a bug. Closing in favour of #26366

@andybalaam
Copy link
Contributor Author

Related: matrix-org/matrix-spec-proposals#3389

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Notifications O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect
Projects
None yet
Development

No branches or pull requests

2 participants