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.

Sign up for GitHub

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

Jump to bottom

Accumulate receipts for the main thread and unthreaded separately #3339

Merged
merged 1 commit into from
May 2, 2023
Merged

Accumulate receipts for the main thread and unthreaded separately #3339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions spec/unit/receipt-accumulator.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,50 @@ describe("ReceiptAccumulator", function () {
]),
);
});

it("Keeps main thread receipts even when an unthreaded receipt came later", () => {
const acc = new ReceiptAccumulator();

// Given receipts for the special thread "main" and also unthreaded
// receipts (which have no thread id).
const receipt1 = newReceipt("$event1", ReceiptType.Read, "@alice:localhost", 1, "main");
const receipt2 = newReceipt("$event2", ReceiptType.Read, "@alice:localhost", 2);

// When we collect them
acc.consumeEphemeralEvents([receipt1, receipt2]);
const newEvent = acc.buildAccumulatedReceiptEvent(roomId);

// We preserve both: thread:main and unthreaded receipts are different
// things, with different meanings.
expect(newEvent).toEqual(
newMultiReceipt([
["$event1", ReceiptType.Read, "@alice:localhost", 1, "main"],
["$event2", ReceiptType.Read, "@alice:localhost", 2, undefined],
]),
);
});

it("Keeps unthreaded receipts even when a main thread receipt came later", () => {
const acc = new ReceiptAccumulator();

// Given receipts for the special thread "main" and also unthreaded
// receipts (which have no thread id).
const receipt1 = newReceipt("$event1", ReceiptType.Read, "@alice:localhost", 1);
const receipt2 = newReceipt("$event2", ReceiptType.Read, "@alice:localhost", 2, "main");

// When we collect them
acc.consumeEphemeralEvents([receipt1, receipt2]);
const newEvent = acc.buildAccumulatedReceiptEvent(roomId);

// We preserve both: thread:main and unthreaded receipts are different
// things, with different meanings.
expect(newEvent).toEqual(
newMultiReceipt([
["$event1", ReceiptType.Read, "@alice:localhost", 1, undefined],
["$event2", ReceiptType.Read, "@alice:localhost", 2, "main"],
]),
);
});
});

const newReceipt = (
Expand Down
4 changes: 2 additions & 2 deletions src/receipt-accumulator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { IMinimalEvent } from "./sync-accumulator";
import { EventType } from "./@types/event";
import { isSupportedReceiptType, MapWithDefault, recursiveMapToObject } from "./utils";
import { IContent } from "./models/event";
import { MAIN_ROOM_TIMELINE, ReceiptContent, ReceiptType } from "./@types/read_receipts";
import { ReceiptContent, ReceiptType } from "./@types/read_receipts";

interface AccumulatedReceipt {
data: IMinimalEvent;
Expand Down Expand Up @@ -118,7 +118,7 @@ export class ReceiptAccumulator {
eventId,
};

if (!data.thread_id || data.thread_id === MAIN_ROOM_TIMELINE) {
Copy link
Contributor

@MadLittleMods MadLittleMods May 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

A comment with some context would be helpful why "we shouldn't also be checking for MAIN_ROOM_TIMELINE because ... otherwise we run into bug x. We want to avoid making messages unread after ..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about it, but basically the old code used to treat the main thread as special, and the fix is not to, so it felt like the comment should have been there before, and my change would have deleted it...

Copy link
Contributor

Choose a reason for hiding this comment

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

MAIN_ROOM_TIMELINE is still a thing in the codebase that people need to reason about and I'm confused and have to think about whenever I look at it.

  • A normal message is part of thread_id = MAIN_ROOM_TIMELINE
  • A thread root is part of thread_id = MAIN_ROOM_TIMELINE
  • A threaded reply has a thread_id = 'xxx'

Perhaps we can add this in a comment somewhere:

In a world that supports threads, all read receipts have a thread_id which is either the thread they belong in or MAIN_ROOM_TIMELINE and we should always use setThreaded(...) here. The MAIN_ROOM_TIMELINE is just treated as another thread.

People still may send us read receipts from clients that don't support threads (unthreaded, without the thread_id property) which we use setUnthreaded(...) for.

Using the wrong method will cause undefined behavior like messages re-appearing as "new" when you already read them previously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not quite it - when the user marks a room as read e.g. by pressing Esc or clicking the "x" in the top right, we do send an unthreaded receipt. I'd happily merge a PR from you with a comment like this somewhere appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Created #3378 and incorporated those caveats 👍

if (!data.thread_id) {
this.setUnthreaded(userId, receipt);
} else {
this.setThreaded(data.thread_id, userId, receipt);
Expand Down