fix: Make read receipts actually work and not explode everywhere #2481

Merged
Jade merged 9 commits from jade/compliant-read-receipts into main 2026-11-10 20:02:47 +00:00
Owner

This is experimental working well on ellis.link

Closes #2436
Closes #741 ?

This is ~~experimental~~ working well on ellis.link Closes #2436 Closes #741 ?
test: Add unit test for merging threaded and private receipts
Some checks failed
Auto Labeler / Apply labels based on changed files (pull_request_target) Successful in 4s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m4s
Checks / Prek / Check changed files (pull_request) Successful in 6s
Checks / Changelog / Check changelog is added (pull_request_target) Failing after 6s
Documentation / Build and Deploy Documentation (pull_request) Failing after 5m23s
Checks / Prek / Clippy and Cargo Tests (pull_request) Failing after 9m13s
c89ce0bbef
@ -59,0 +67,4 @@
let mut is_dup = false;
for key in keys {
if let Ok(val) = self.readreceiptid_readreceipt.get(&key).await {
Author
Owner

I hate this code, might rewrite

I hate this code, might rewrite
Jade marked this conversation as resolved
Lines 39-41
@ -30,0 +36,6 @@
return Err!(Request(Forbidden("You are not in this room.")));
}
if services.users.is_suspended(sender_user).await? {
return Err!(Request(Forbidden("User is suspended.")));
}
Owner

You should still be allowed to mark messages as read even while suspended, otherwise notification counts and unread counts will just rise forever until the user is unsuspended

You should still be allowed to mark messages as read even while suspended, otherwise notification counts and unread counts will just rise forever until the user is unsuspended
Owner

Technically /redact is also supposed to let you redact your own messages while suspended, I was just too lazy to special case self-redacts

Technically /redact is also supposed to let you redact your own messages while suspended, I was just too lazy to special case self-redacts
Jade marked this conversation as resolved
Author
Owner

Well this is running on production well ✅

Well this is running on production well ✅
Jade force-pushed jade/compliant-read-receipts from c89ce0bbef
Some checks failed
Auto Labeler / Apply labels based on changed files (pull_request_target) Successful in 4s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m4s
Checks / Prek / Check changed files (pull_request) Successful in 6s
Checks / Changelog / Check changelog is added (pull_request_target) Failing after 6s
Documentation / Build and Deploy Documentation (pull_request) Failing after 5m23s
Checks / Prek / Clippy and Cargo Tests (pull_request) Failing after 9m13s
to 66f320171e
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Failing after 7s
Checks / Prek / Check changed files (pull_request) Successful in 8s
Documentation / Build and Deploy Documentation (pull_request) Failing after 44s
Checks / Prek / Pre-commit & Formatting (pull_request) Has been cancelled
Checks / Prek / Clippy and Cargo Tests (pull_request) Has been cancelled
2026-11-09 23:39:44 +00:00
Compare
Jade force-pushed jade/compliant-read-receipts from 66f320171e
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Failing after 7s
Checks / Prek / Check changed files (pull_request) Successful in 8s
Documentation / Build and Deploy Documentation (pull_request) Failing after 44s
Checks / Prek / Pre-commit & Formatting (pull_request) Has been cancelled
Checks / Prek / Clippy and Cargo Tests (pull_request) Has been cancelled
to 9abbcc6f77
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Failing after 8s
Checks / Prek / Check changed files (pull_request) Successful in 6s
Update flake hashes / update-flake-hashes (pull_request) Successful in 1m23s
Documentation / Build and Deploy Documentation (pull_request) Failing after 5m29s
Checks / Prek / Clippy and Cargo Tests (pull_request) Failing after 8m5s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 21m4s
2026-11-09 23:43:23 +00:00
Compare
Jade requested review from Owners 2026-11-10 00:43:57 +00:00
Author
Owner

sidenote, might drop c17e789f9d because I'm not actually sure it's an improvement in UX in element x-based apps at least

sidenote, might drop c17e789f9d25da2ff1bfe2b1c7ee05e63e731f8d because I'm not actually sure it's an improvement in UX in element x-based apps at least
ginger added this to the 26.11.0 milestone 2026-11-10 16:18:13 +00:00
Jade force-pushed jade/compliant-read-receipts from 9abbcc6f77
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Failing after 8s
Checks / Prek / Check changed files (pull_request) Successful in 6s
Update flake hashes / update-flake-hashes (pull_request) Successful in 1m23s
Documentation / Build and Deploy Documentation (pull_request) Failing after 5m29s
Checks / Prek / Clippy and Cargo Tests (pull_request) Failing after 8m5s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 21m4s
to 27de9f58a4
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Failing after 7s
Documentation / Build and Deploy Documentation (pull_request) Failing after 42s
Checks / Prek / Check changed files (pull_request) Successful in 8s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m14s
Checks / Prek / Clippy and Cargo Tests (pull_request) Has been cancelled
2026-11-10 16:32:47 +00:00
Compare
Jade force-pushed jade/compliant-read-receipts from 27de9f58a4
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Failing after 7s
Documentation / Build and Deploy Documentation (pull_request) Failing after 42s
Checks / Prek / Check changed files (pull_request) Successful in 8s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m14s
Checks / Prek / Clippy and Cargo Tests (pull_request) Has been cancelled
to f75c9a9739
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Failing after 10s
Documentation / Build and Deploy Documentation (pull_request) Failing after 42s
Checks / Prek / Check changed files (pull_request) Successful in 8s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m16s
Checks / Prek / Clippy and Cargo Tests (pull_request) Has been cancelled
2026-11-10 16:34:54 +00:00
Compare
Author
Owner

Dropped ✅

Dropped ✅
ginger requested changes 2026-11-10 16:36:56 +00:00
Dismissed
ginger left a comment

I think the force push broke this review but we'll see what happens

I think the force push broke this review but we'll see what happens
@ -48,2 +48,2 @@
) {
// Remove old entry
) -> bool {
let new_target = event.content.0.iter().find_map(|(ev_id, types)| {
Owner

I think this could be a call to user_receipt?

I _think_ this could be a call to [`user_receipt`](https://docs.rs/ruma/latest/ruma/events/receipt/struct.ReceiptEventContent.html#method.user_receipt)?
Author
Owner

Well, that does save a couple LOC

Well, that does save a couple LOC
Jade marked this conversation as resolved
@ -59,0 +69,4 @@
for key in keys {
if let Ok(val) = self.readreceiptid_readreceipt.get(&key).await {
if let Ok(existing) = serde_json::from_slice::<ReceiptEvent>(&val) {
let existing_target = existing.content.0.iter().find_map(|(ev_id, types)| {
Owner

ReceiptEventContent derefs to its inner BTreeMap, you don't need the .0 here (also this could probably be a user_receipt call)

`ReceiptEventContent` derefs to its inner `BTreeMap`, you don't need the `.0` here (also this could probably be a `user_receipt` call)
Jade marked this conversation as resolved
@ -59,0 +75,4 @@
})
});
if let (Some(new_t), Some(old_t)) = (new_target, existing_target) {
Owner

begging you to just shadow new_target and existing_target instead of chopping letters off the variable names

begging you to just shadow `new_target` and `existing_target` instead of chopping letters off the variable names
Author
Owner

How about I just kill that entirely

How about I just kill that entirely
Jade marked this conversation as resolved
@ -59,0 +76,4 @@
});
if let (Some(new_t), Some(old_t)) = (new_target, existing_target) {
if new_t.1 == old_t.1 && new_t.2 == old_t.2 {
Owner

these tuple accesses are not at all easy to understand without the benefit of an IDE

these tuple accesses are not at all easy to understand without the benefit of an IDE
Jade marked this conversation as resolved
@ -59,0 +79,4 @@
if new_t.1 == old_t.1 && new_t.2 == old_t.2 {
if new_t.0 == old_t.0 {
is_dup = true;
} else {
Owner

surely there's a way to do this that doesn't involve four consecutive else cases with the same content?

surely there's a way to do this that doesn't involve four consecutive `else` cases with the same content?
Author
Owner

I improved it a bit 🥺

I improved it a bit 🥺
ginger marked this conversation as resolved
chore: Add changelog
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 7s
Documentation / Build and Deploy Documentation (pull_request) Failing after 43s
Checks / Prek / Check changed files (pull_request) Successful in 8s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m15s
Checks / Prek / Clippy and Cargo Tests (pull_request) Failing after 8m12s
1f16f68241
refactor(rooms/timeline): Simplify deduplication of read receipts
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 7s
Checks / Prek / Check changed files (pull_request) Successful in 7s
Documentation / Build and Deploy Documentation (pull_request) Failing after 40s
Checks / Prek / Clippy and Cargo Tests (pull_request) Failing after 8m17s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 21m11s
527772eb83
ginger approved these changes 2026-11-10 17:34:56 +00:00
Jade force-pushed jade/compliant-read-receipts from 527772eb83
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 7s
Checks / Prek / Check changed files (pull_request) Successful in 7s
Documentation / Build and Deploy Documentation (pull_request) Failing after 40s
Checks / Prek / Clippy and Cargo Tests (pull_request) Failing after 8m17s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 21m11s
to 88441a409a
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 7s
Checks / Prek / Check changed files (pull_request) Successful in 8s
Documentation / Build and Deploy Documentation (pull_request) Successful in 1m1s
Update flake hashes / update-flake-hashes (pull_request) Successful in 1m21s
Documentation / Build and Deploy Documentation (push) Successful in 1m3s
Checks / Prek / Check changed files (push) Successful in 8s
Checks / Prek / Pre-commit & Formatting (push) Successful in 1m12s
Release Docker Image / Build linux-amd64 (release) (push) Failing after 3m18s
Release Docker Image / Build linux-arm64 (release) (push) Has been cancelled
Release Docker Image / Create Multi-arch Release Manifest (push) Has been cancelled
Release Docker Image / Build linux-amd64 (max-perf) (push) Has been cancelled
Release Docker Image / Build linux-arm64 (max-perf) (push) Has been cancelled
Release Docker Image / Create Max-Perf Manifest (push) Has been cancelled
Release Docker Image / Release Binaries (push) Has been cancelled
Release Docker Image / Mirror Images (push) Has been cancelled
Checks / Prek / Clippy and Cargo Tests (push) Successful in 8m56s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 21m8s
Checks / Prek / Clippy and Cargo Tests (pull_request) Has been cancelled
2026-11-10 20:02:33 +00:00
Compare
Jade merged commit 88441a409a into main 2026-11-10 20:02:47 +00:00
Jade deleted branch jade/compliant-read-receipts 2026-11-10 20:02:47 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
continuwuation/continuwuity!2481
No description provided.