feat(timeline): send timeline redactions through the send queue#6428
feat(timeline): send timeline redactions through the send queue#6428Johennes wants to merge 1 commit into
Conversation
14d5ef3 to
c5effdc
Compare
|
@bnjbvr I'm curious what you think of the API shape here? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6428 +/- ##
==========================================
+ Coverage 89.95% 89.97% +0.02%
==========================================
Files 382 382
Lines 107979 107984 +5
Branches 107979 107984 +5
==========================================
+ Hits 97132 97161 +29
+ Misses 7177 7162 -15
+ Partials 3670 3661 -9 ☔ View full report in Codecov by Sentry. |
Thanks for making this a draft. Honestly we're already sending all events via the send queue, so I wouldn't use a feature toggle ( |
c5effdc to
7837d99
Compare
| assert_let_timeout!(Some(timeline_updates) = timeline_stream.next()); | ||
| assert_eq!(timeline_updates.len(), 1); | ||
|
|
||
| assert_let!(VectorDiff::Set { index: 1, value: item } = &timeline_updates[0]); | ||
| assert!(item.as_event().unwrap().content().is_redacted()); | ||
|
|
||
| assert_let_timeout!(Some(timeline_updates) = timeline_stream.next()); | ||
| assert_eq!(timeline_updates.len(), 2); | ||
|
|
||
| assert_let!(VectorDiff::Set { index: 1, value: item } = &timeline_updates[0]); | ||
| assert!(item.as_event().unwrap().content().is_redacted()); | ||
| assert_let!(VectorDiff::Set { index: 1, value: item } = &timeline_updates[1]); | ||
| assert!(item.as_event().unwrap().content().is_redacted()); | ||
|
|
There was a problem hiding this comment.
I'm unsure why this is issued three times instead of just twice (local and remote echo of the redaction event). 🤔
There was a problem hiding this comment.
I got curious about this, and found that all three items have the same "unique" ID, 0.
The first one looks like this:
TimelineItem {
kind: Event(
EventTimelineItem {
sender: "@a:b.com",
...
timestamp: 2026-04-15T09:12:45.439,
content: MsgLike(
MsgLikeContent {
kind: Redacted,
...
unredacted_item: Some(
UnredactedEventTimelineItem {
...
{"content":{"body":"buy my bitcoins bro","msgtype":"m.text"},"event_id":"$kZVJhqxRxGeHOpPwPTu2a9i00EiKLmYNtr4JgFaFxck","origin_server_ts":1776244365439,"sender":"@a:b.com","type":"m.room.message"},
...
kind: Remote(
RemoteEventTimelineItem {
event_id: "$kZVJhqxRxGeHOpPwPTu2a9i00EiKLmYNtr4JgFaFxck",
...
internal_id: TimelineUniqueId(
"0",
),
}
and the second two are identical (at least in terms of debug formatting) and look like:
TimelineItem {
kind: Event(
EventTimelineItem {
sender: "@a:b.com",
...
timestamp: 2026-04-15T09:12:45.439,
content: MsgLike(
MsgLikeContent {
kind: Redacted,
...
unredacted_item: None,
kind: Remote(
RemoteEventTimelineItem {
event_id: "$kZVJhqxRxGeHOpPwPTu2a9i00EiKLmYNtr4JgFaFxck",
...
internal_id: TimelineUniqueId(
"0",
),
}
There was a problem hiding this comment.
There should indeed be only two (one for the local echo, one after it's been sent by the send queue it should be saved in the event cache). If there was a fake sync mimicking the remote echo, that would explain the third one, but it's not clear here why there's a duplicate one. Wondering if this is a spurious redaction aggregation applied to the redacted item itself, or something like that 👀 If you're interested in digging deeper, that might help avoiding spurious updates.
There was a problem hiding this comment.
@Johennes ideally we would work this out before merging, but since I suspect it is unrelated to your change, I could be persuaded to merge without if no-one else on the Rust team objects.
There was a problem hiding this comment.
I spent some time looking into this but so far couldn't figure out where the third update comes from, unfortunately. 😕
There was a problem hiding this comment.
I am going to defer to @poljar on whether it is ok to merge this without understanding why this code is not behaving how we think it should.
7837d99 to
4458835
Compare
andybalaam
left a comment
There was a problem hiding this comment.
Looks good to me, but want to resolve the question you raised before merging. Thanks!
| assert_let_timeout!(Some(timeline_updates) = timeline_stream.next()); | ||
| assert_eq!(timeline_updates.len(), 1); | ||
|
|
||
| assert_let!(VectorDiff::Set { index: 1, value: item } = &timeline_updates[0]); | ||
| assert!(item.as_event().unwrap().content().is_redacted()); | ||
|
|
||
| assert_let_timeout!(Some(timeline_updates) = timeline_stream.next()); | ||
| assert_eq!(timeline_updates.len(), 2); | ||
|
|
||
| assert_let!(VectorDiff::Set { index: 1, value: item } = &timeline_updates[0]); | ||
| assert!(item.as_event().unwrap().content().is_redacted()); | ||
| assert_let!(VectorDiff::Set { index: 1, value: item } = &timeline_updates[1]); | ||
| assert!(item.as_event().unwrap().content().is_redacted()); | ||
|
|
There was a problem hiding this comment.
I got curious about this, and found that all three items have the same "unique" ID, 0.
The first one looks like this:
TimelineItem {
kind: Event(
EventTimelineItem {
sender: "@a:b.com",
...
timestamp: 2026-04-15T09:12:45.439,
content: MsgLike(
MsgLikeContent {
kind: Redacted,
...
unredacted_item: Some(
UnredactedEventTimelineItem {
...
{"content":{"body":"buy my bitcoins bro","msgtype":"m.text"},"event_id":"$kZVJhqxRxGeHOpPwPTu2a9i00EiKLmYNtr4JgFaFxck","origin_server_ts":1776244365439,"sender":"@a:b.com","type":"m.room.message"},
...
kind: Remote(
RemoteEventTimelineItem {
event_id: "$kZVJhqxRxGeHOpPwPTu2a9i00EiKLmYNtr4JgFaFxck",
...
internal_id: TimelineUniqueId(
"0",
),
}
and the second two are identical (at least in terms of debug formatting) and look like:
TimelineItem {
kind: Event(
EventTimelineItem {
sender: "@a:b.com",
...
timestamp: 2026-04-15T09:12:45.439,
content: MsgLike(
MsgLikeContent {
kind: Redacted,
...
unredacted_item: None,
kind: Remote(
RemoteEventTimelineItem {
event_id: "$kZVJhqxRxGeHOpPwPTu2a9i00EiKLmYNtr4JgFaFxck",
...
internal_id: TimelineUniqueId(
"0",
),
}
| assert_let_timeout!(Some(timeline_updates) = timeline_stream.next()); | ||
| assert_eq!(timeline_updates.len(), 1); | ||
|
|
||
| assert_let!(VectorDiff::Set { index: 1, value: item } = &timeline_updates[0]); | ||
| assert!(item.as_event().unwrap().content().is_redacted()); | ||
|
|
||
| assert_let_timeout!(Some(timeline_updates) = timeline_stream.next()); | ||
| assert_eq!(timeline_updates.len(), 2); | ||
|
|
||
| assert_let!(VectorDiff::Set { index: 1, value: item } = &timeline_updates[0]); | ||
| assert!(item.as_event().unwrap().content().is_redacted()); | ||
| assert_let!(VectorDiff::Set { index: 1, value: item } = &timeline_updates[1]); | ||
| assert!(item.as_event().unwrap().content().is_redacted()); | ||
|
|
Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
4458835 to
daad851
Compare
There was a problem hiding this comment.
How the app is supposed to display a redaction failed to be sent? I'm asking because the Latest Event can reflect error from the Send Queue, but if the Timeline isn't able to render a redaction failed to be sent, it's gonna be super confusing for the user.
| @@ -0,0 +1 @@ | |||
| [**breaking**] Send redactions issued via `Timeline::redact_event` through the send queue. | |||
There was a problem hiding this comment.
The API shape is technically the same. However, redact.await will no longer mean that the redaction has been sent but instead just that it has been enqueued in the send queue. Callers will have to monitor send queue updates if they're interested in the sending state. That's why I thought it would qualify as a breaking change.
This makes it possible to send redactions via the send queue using
Timeline::redact.CHANGELOG.mdfiles.