Skip to content

Commit

Permalink
Fix synthetic read receipt handling (#2174)
Browse files Browse the repository at this point in the history
  • Loading branch information
@t3chguy
t3chguy authored Feb 15, 2022
1 parent 2bb1469 commit 2910e62
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 18 deletions.
45 changes: 45 additions & 0 deletions spec/unit/room.spec.js
View file
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
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 @@ -1981,10 +1982,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 @@ -2048,10 +2050,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 @@ -2060,9 +2062,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 @@ -2079,15 +2081,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 @@ -2098,8 +2102,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 @@ -2124,14 +2156,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

0 comments on commit 2910e62

Please sign in to comment.