Skip to content

feat(timeline): send timeline redactions through the send queue#6428

Open
Johennes wants to merge 1 commit into
matrix-org:mainfrom
Johennes:johannes/timeline-redactions-send-queue
Open

feat(timeline): send timeline redactions through the send queue#6428
Johennes wants to merge 1 commit into
matrix-org:mainfrom
Johennes:johannes/timeline-redactions-send-queue

Conversation

@Johennes

@Johennes Johennes commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

This makes it possible to send redactions via the send queue using Timeline::redact.

  • I've documented the public API Changes in the appropriate CHANGELOG.md files.
  • This PR was made with the help of AI.

@Johennes Johennes force-pushed the johannes/timeline-redactions-send-queue branch 3 times, most recently from 14d5ef3 to c5effdc Compare April 9, 2026 15:24
@Johennes

Johennes commented Apr 9, 2026

Copy link
Copy Markdown
Contributor Author

@bnjbvr I'm curious what you think of the API shape here?

@codecov

codecov Bot commented Apr 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.97%. Comparing base (3a9fd9f) to head (daad851).
⚠️ Report is 5 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
crates/matrix-sdk-ui/src/timeline/mod.rs 85.71% 0 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

@codspeed-hq

codspeed-hq Bot commented Apr 9, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 50 untouched benchmarks


Comparing Johennes:johannes/timeline-redactions-send-queue (daad851) with main (3a9fd9f)

Open in CodSpeed

@bnjbvr

bnjbvr commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

@bnjbvr I'm curious what you think of the API shape here?

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 (use_send_queue) for sending redactions, and just use the send queue all the time. (Not sure if the redaction future would be useful in this case; I'm fine with having one, if needs be!)

@Johennes Johennes changed the title feat(timeline): optionally send redactions through the send queue feat(timeline): send timeline redactions through the send queue Apr 14, 2026
@Johennes Johennes force-pushed the johannes/timeline-redactions-send-queue branch from c5effdc to 7837d99 Compare April 14, 2026 15:35
Comment on lines +448 to +461
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());

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure why this is issued three times instead of just twice (local and remote echo of the redaction event). 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",
    ),
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether @bnjbvr @Hywan might know whether this is expected?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent some time looking into this but so far couldn't figure out where the third update comes from, unfortunately. 😕

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Johennes Johennes force-pushed the johannes/timeline-redactions-send-queue branch from 7837d99 to 4458835 Compare April 14, 2026 15:37
@Johennes Johennes marked this pull request as ready for review April 14, 2026 15:49
@Johennes Johennes requested a review from a team as a code owner April 14, 2026 15:49
@Johennes Johennes requested review from andybalaam and removed request for a team April 14, 2026 15:49

@andybalaam andybalaam left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but want to resolve the question you raised before merging. Thanks!

Comment on lines +448 to +461
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());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",
    ),
}

Comment on lines +448 to +461
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());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether @bnjbvr @Hywan might know whether this is expected?

Comment thread bindings/matrix-sdk-ffi/CHANGELOG.md Outdated
Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
@Johennes Johennes force-pushed the johannes/timeline-redactions-send-queue branch from 4458835 to daad851 Compare June 1, 2026 18:48
@Johennes Johennes requested a review from andybalaam June 1, 2026 19:17

@Hywan Hywan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

cc @manuroe @mxandreas

@@ -0,0 +1 @@
[**breaking**] Send redactions issued via `Timeline::redact_event` through the send queue.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it a breaking change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants