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

Fix synthetic read receipt handling #2183

Merged
merged 1 commit into from
Feb 17, 2022
Merged

Fix synthetic read receipt handling #2183

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
45 changes: 45 additions & 0 deletions spec/unit/room.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1143,6 +1143,51 @@ describe("Room", function() {
]));
expect(room.getEventReadUpTo(userB)).toEqual(events[2].getId());
});

it("should prioritise the most recent event even if it is synthetic", () => {
const events = [
utils.mkMessage({
room: roomId, user: userA, msg: "1111",
event: true,
}),
utils.mkMessage({
room: roomId, user: userA, msg: "2222",
event: true,
}),
utils.mkMessage({
room: roomId, user: userA, msg: "3333",
event: true,
}),
];

room.addLiveEvents(events);
const ts = 13787898424;

// check it initialises correctly
room.addReceipt(mkReceipt(roomId, [
mkRecord(events[0].getId(), "m.read", userB, ts),
]));
expect(room.getEventReadUpTo(userB)).toEqual(events[0].getId());

// 2>0, so it should move forward
room.addReceipt(mkReceipt(roomId, [
mkRecord(events[2].getId(), "m.read", userB, ts),
]), true);
expect(room.getEventReadUpTo(userB)).toEqual(events[2].getId());
expect(room.getReceiptsForEvent(events[2])).toEqual([
{ data: { ts }, type: "m.read", userId: userB },
]);

// 1<2, so it should stay put
room.addReceipt(mkReceipt(roomId, [
mkRecord(events[1].getId(), "m.read", userB, ts),
]));
expect(room.getEventReadUpTo(userB)).toEqual(events[2].getId());
expect(room.getEventReadUpTo(userB, true)).toEqual(events[1].getId());
expect(room.getReceiptsForEvent(events[2])).toEqual([
{ data: { ts }, type: "m.read", userId: userB },
]);
});
});

describe("getUsersReadUpTo", function() {
Expand Down
60 changes: 42 additions & 18 deletions src/models/room.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ interface IReceiptContent {

const ReceiptPairRealIndex = 0;
const ReceiptPairSyntheticIndex = 1;
// We will only hold a synthetic receipt if we do not have a real receipt or the synthetic is newer.
type Receipts = {
[receiptType: string]: {
[userId: string]: [IWrappedReceipt, IWrappedReceipt]; // Pair<real receipt, synthetic receipt> (both nullable)
Expand Down Expand Up @@ -1986,10 +1987,11 @@ export class Room extends EventEmitter {

public getReadReceiptForUserId(userId: string, ignoreSynthesized = false): IWrappedReceipt | null {
const [realReceipt, syntheticReceipt] = this.receipts["m.read"]?.[userId] ?? [];
if (ignoreSynthesized || realReceipt) {
if (ignoreSynthesized) {
return realReceipt;
}
return syntheticReceipt;

return syntheticReceipt ?? realReceipt;
}

/**
Expand Down Expand Up @@ -2053,10 +2055,10 @@ export class Room extends EventEmitter {
/**
* Add a receipt event to the room.
* @param {MatrixEvent} event The m.receipt event.
* @param {Boolean} fake True if this event is implicit.
* @param {Boolean} synthetic True if this event is implicit.
*/
public addReceipt(event: MatrixEvent, fake = false): void {
this.addReceiptsToStructure(event, fake);
public addReceipt(event: MatrixEvent, synthetic = false): void {
this.addReceiptsToStructure(event, synthetic);
// send events after we've regenerated the structure & cache, otherwise things that
// listened for the event would read stale data.
this.emit("Room.receipt", event, this);
Expand All @@ -2065,9 +2067,9 @@ export class Room extends EventEmitter {
/**
* Add a receipt event to the room.
* @param {MatrixEvent} event The m.receipt event.
* @param {Boolean} fake True if this event is implicit.
* @param {Boolean} synthetic True if this event is implicit.
*/
private addReceiptsToStructure(event: MatrixEvent, fake: boolean): void {
private addReceiptsToStructure(event: MatrixEvent, synthetic: boolean): void {
const content = event.getContent<IReceiptContent>();
Object.keys(content).forEach((eventId) => {
Object.keys(content[eventId]).forEach((receiptType) => {
Expand All @@ -2084,15 +2086,17 @@ export class Room extends EventEmitter {
const pair = this.receipts[receiptType][userId];

let existingReceipt = pair[ReceiptPairRealIndex];
if (fake && !existingReceipt) {
existingReceipt = pair[ReceiptPairSyntheticIndex];
if (synthetic) {
existingReceipt = pair[ReceiptPairSyntheticIndex] ?? pair[ReceiptPairRealIndex];
}

if (existingReceipt) {
// we only want to add this receipt if we think it is later than the one we already have.
// This is managed server-side, but because we synthesize RRs locally we have to do it here too.
const ordering = this.getUnfilteredTimelineSet().compareEventOrdering(
existingReceipt.eventId, eventId);
existingReceipt.eventId,
eventId,
);
if (ordering !== null && ordering >= 0) {
return;
}
Expand All @@ -2103,8 +2107,36 @@ export class Room extends EventEmitter {
data: receipt,
};

const realReceipt = synthetic ? pair[ReceiptPairRealIndex] : wrappedReceipt;
const syntheticReceipt = synthetic ? wrappedReceipt : pair[ReceiptPairSyntheticIndex];

let ordering: number | null = null;
if (realReceipt && syntheticReceipt) {
ordering = this.getUnfilteredTimelineSet().compareEventOrdering(
realReceipt.eventId,
syntheticReceipt.eventId,
);
}

const preferSynthetic = ordering === null || ordering < 0;

// we don't bother caching just real receipts by event ID as there's nothing that would read it.
// Take the current cached receipt before we overwrite the pair elements.
const cachedReceipt = pair[ReceiptPairSyntheticIndex] ?? pair[ReceiptPairRealIndex];

if (synthetic && preferSynthetic) {
pair[ReceiptPairSyntheticIndex] = wrappedReceipt;
} else if (!synthetic) {
pair[ReceiptPairRealIndex] = wrappedReceipt;

if (!preferSynthetic) {
pair[ReceiptPairSyntheticIndex] = null;
}
}

const newCachedReceipt = pair[ReceiptPairSyntheticIndex] ?? pair[ReceiptPairRealIndex];
if (cachedReceipt === newCachedReceipt) return;

// clean up any previous cache entry
if (cachedReceipt && this.receiptCacheByEventId[cachedReceipt.eventId]) {
const previousEventId = cachedReceipt.eventId;
Expand All @@ -2129,14 +2161,6 @@ export class Room extends EventEmitter {
type: receiptType,
data: receipt,
});

if (fake) {
pair[ReceiptPairSyntheticIndex] = wrappedReceipt;
} else {
pair[ReceiptPairRealIndex] = wrappedReceipt;
// a real receipt for a receiptType+userId tuple should clobber any synthetic one
pair[ReceiptPairSyntheticIndex] = null;
}
});
});
});
Expand Down