Skip to content

Commit 6316a6a

Browse files
SimonBrandnerturt2live
SimonBrandner
and
turt2live
authored
Add support for stable prefixes for MSC2285 (#2524)
Co-authored-by: Travis Ralston <[email protected]>
1 parent 575b416 commit 6316a6a

File tree

8 files changed

+243
-55
lines changed

8 files changed

+243
-55
lines changed

‎spec/unit/room.spec.ts‎

Lines changed: 89 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2435,16 +2435,96 @@ describe("Room", function() {
24352435
expect(room.getEventReadUpTo(userA)).toEqual("eventId");
24362436
});
24372437

2438-
it("prefers older receipt", () => {
2439-
room.getReadReceiptForUserId = (userId, ignore, receiptType) => {
2440-
return (receiptType === ReceiptType.Read
2441-
? { eventId: "eventId1" }
2442-
: { eventId: "eventId2" }
2443-
) as IWrappedReceipt;
2444-
};
2445-
room.getUnfilteredTimelineSet = () => ({ compareEventOrdering: (event1, event2) => 1 } as EventTimelineSet);
2438+
describe("prefers newer receipt", () => {
2439+
it("should compare correctly using timelines", () => {
2440+
room.getReadReceiptForUserId = (userId, ignore, receiptType) => {
2441+
if (receiptType === ReceiptType.ReadPrivate) {
2442+
return { eventId: "eventId1" } as IWrappedReceipt;
2443+
}
2444+
if (receiptType === ReceiptType.UnstableReadPrivate) {
2445+
return { eventId: "eventId2" } as IWrappedReceipt;
2446+
}
2447+
if (receiptType === ReceiptType.Read) {
2448+
return { eventId: "eventId3" } as IWrappedReceipt;
2449+
}
2450+
};
2451+
2452+
for (let i = 1; i <= 3; i++) {
2453+
room.getUnfilteredTimelineSet = () => ({ compareEventOrdering: (event1, event2) => {
2454+
return (event1 === `eventId${i}`) ? 1 : -1;
2455+
} } as EventTimelineSet);
2456+
2457+
expect(room.getEventReadUpTo(userA)).toEqual(`eventId${i}`);
2458+
}
2459+
});
2460+
2461+
it("should compare correctly by timestamp", () => {
2462+
for (let i = 1; i <= 3; i++) {
2463+
room.getUnfilteredTimelineSet = () => ({
2464+
compareEventOrdering: (_1, _2) => null,
2465+
} as EventTimelineSet);
2466+
room.getReadReceiptForUserId = (userId, ignore, receiptType) => {
2467+
if (receiptType === ReceiptType.ReadPrivate) {
2468+
return { eventId: "eventId1", data: { ts: i === 1 ? 1 : 0 } } as IWrappedReceipt;
2469+
}
2470+
if (receiptType === ReceiptType.UnstableReadPrivate) {
2471+
return { eventId: "eventId2", data: { ts: i === 2 ? 1 : 0 } } as IWrappedReceipt;
2472+
}
2473+
if (receiptType === ReceiptType.Read) {
2474+
return { eventId: "eventId3", data: { ts: i === 3 ? 1 : 0 } } as IWrappedReceipt;
2475+
}
2476+
};
2477+
2478+
expect(room.getEventReadUpTo(userA)).toEqual(`eventId${i}`);
2479+
}
2480+
});
24462481

2447-
expect(room.getEventReadUpTo(userA)).toEqual("eventId1");
2482+
describe("fallback precedence", () => {
2483+
beforeAll(() => {
2484+
room.getUnfilteredTimelineSet = () => ({
2485+
compareEventOrdering: (_1, _2) => null,
2486+
} as EventTimelineSet);
2487+
});
2488+
2489+
it("should give precedence to m.read.private", () => {
2490+
room.getReadReceiptForUserId = (userId, ignore, receiptType) => {
2491+
if (receiptType === ReceiptType.ReadPrivate) {
2492+
return { eventId: "eventId1" } as IWrappedReceipt;
2493+
}
2494+
if (receiptType === ReceiptType.UnstableReadPrivate) {
2495+
return { eventId: "eventId2" } as IWrappedReceipt;
2496+
}
2497+
if (receiptType === ReceiptType.Read) {
2498+
return { eventId: "eventId3" } as IWrappedReceipt;
2499+
}
2500+
};
2501+
2502+
expect(room.getEventReadUpTo(userA)).toEqual(`eventId1`);
2503+
});
2504+
2505+
it("should give precedence to org.matrix.msc2285.read.private", () => {
2506+
room.getReadReceiptForUserId = (userId, ignore, receiptType) => {
2507+
if (receiptType === ReceiptType.UnstableReadPrivate) {
2508+
return { eventId: "eventId2" } as IWrappedReceipt;
2509+
}
2510+
if (receiptType === ReceiptType.Read) {
2511+
return { eventId: "eventId2" } as IWrappedReceipt;
2512+
}
2513+
};
2514+
2515+
expect(room.getEventReadUpTo(userA)).toEqual(`eventId2`);
2516+
});
2517+
2518+
it("should give precedence to m.read", () => {
2519+
room.getReadReceiptForUserId = (userId, ignore, receiptType) => {
2520+
if (receiptType === ReceiptType.Read) {
2521+
return { eventId: "eventId3" } as IWrappedReceipt;
2522+
}
2523+
};
2524+
2525+
expect(room.getEventReadUpTo(userA)).toEqual(`eventId3`);
2526+
});
2527+
});
24482528
});
24492529
});
24502530
});

‎spec/unit/sync-accumulator.spec.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -302,6 +302,9 @@ describe("SyncAccumulator", function() {
302302
[ReceiptType.ReadPrivate]: {
303303
"@dan:localhost": { ts: 4 },
304304
},
305+
[ReceiptType.UnstableReadPrivate]: {
306+
"@matthew:localhost": { ts: 5 },
307+
},
305308
"some.other.receipt.type": {
306309
"@should_be_ignored:localhost": { key: "val" },
307310
},
@@ -347,6 +350,9 @@ describe("SyncAccumulator", function() {
347350
[ReceiptType.ReadPrivate]: {
348351
"@dan:localhost": { ts: 4 },
349352
},
353+
[ReceiptType.UnstableReadPrivate]: {
354+
"@matthew:localhost": { ts: 5 },
355+
},
350356
},
351357
"$event2:localhost": {
352358
[ReceiptType.Read]: {

‎spec/unit/utils.spec.ts‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import {
1515
import { logger } from "../../src/logger";
1616
import { mkMessage } from "../test-utils/test-utils";
1717
import { makeBeaconEvent } from "../test-utils/beacon";
18+
import { ReceiptType } from "../../src/@types/read_receipts";
1819

1920
// TODO: Fix types throughout
2021

@@ -523,4 +524,54 @@ describe("utils", function() {
523524
).toEqual([beaconEvent2, beaconEvent1, beaconEvent3]);
524525
});
525526
});
527+
528+
describe('getPrivateReadReceiptField', () => {
529+
it('should return m.read.private if server supports stable', async () => {
530+
expect(await utils.getPrivateReadReceiptField({
531+
doesServerSupportUnstableFeature: jest.fn().mockImplementation((feature) => {
532+
return feature === "org.matrix.msc2285.stable";
533+
}),
534+
} as any)).toBe(ReceiptType.ReadPrivate);
535+
});
536+
537+
it('should return m.read.private if server supports stable and unstable', async () => {
538+
expect(await utils.getPrivateReadReceiptField({
539+
doesServerSupportUnstableFeature: jest.fn().mockImplementation((feature) => {
540+
return ["org.matrix.msc2285.stable", "org.matrix.msc2285"].includes(feature);
541+
}),
542+
} as any)).toBe(ReceiptType.ReadPrivate);
543+
});
544+
545+
it('should return org.matrix.msc2285.read.private if server supports unstable', async () => {
546+
expect(await utils.getPrivateReadReceiptField({
547+
doesServerSupportUnstableFeature: jest.fn().mockImplementation((feature) => {
548+
return feature === "org.matrix.msc2285";
549+
}),
550+
} as any)).toBe(ReceiptType.UnstableReadPrivate);
551+
});
552+
553+
it('should return none if server does not support either', async () => {
554+
expect(await utils.getPrivateReadReceiptField({
555+
doesServerSupportUnstableFeature: jest.fn().mockResolvedValue(false),
556+
} as any)).toBeFalsy();
557+
});
558+
});
559+
560+
describe('isSupportedReceiptType', () => {
561+
it('should support m.read', () => {
562+
expect(utils.isSupportedReceiptType(ReceiptType.Read)).toBeTruthy();
563+
});
564+
565+
it('should support m.read.private', () => {
566+
expect(utils.isSupportedReceiptType(ReceiptType.ReadPrivate)).toBeTruthy();
567+
});
568+
569+
it('should support org.matrix.msc2285.read.private', () => {
570+
expect(utils.isSupportedReceiptType(ReceiptType.UnstableReadPrivate)).toBeTruthy();
571+
});
572+
573+
it('should not support other receipt types', () => {
574+
expect(utils.isSupportedReceiptType("this is a receipt type")).toBeFalsy();
575+
});
576+
});
526577
});

‎src/@types/read_receipts.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,5 +17,9 @@ limitations under the License.
1717
export enum ReceiptType {
1818
Read = "m.read",
1919
FullyRead = "m.fully_read",
20-
ReadPrivate = "org.matrix.msc2285.read.private"
20+
ReadPrivate = "m.read.private",
21+
/**
22+
* @deprecated Please use the ReadPrivate type when possible. This value may be removed at any time without notice.
23+
*/
24+
UnstableReadPrivate = "org.matrix.msc2285.read.private",
2125
}

‎src/client.ts‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1088,11 +1088,12 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
10881088
// Figure out if we've read something or if it's just informational
10891089
const content = event.getContent();
10901090
const isSelf = Object.keys(content).filter(eid => {
1091-
const read = content[eid][ReceiptType.Read];
1092-
if (read && Object.keys(read).includes(this.getUserId())) return true;
1091+
for (const [key, value] of Object.entries(content[eid])) {
1092+
if (!utils.isSupportedReceiptType(key)) continue;
1093+
if (!value) continue;
10931094

1094-
const readPrivate = content[eid][ReceiptType.ReadPrivate];
1095-
if (readPrivate && Object.keys(readPrivate).includes(this.getUserId())) return true;
1095+
if (Object.keys(value).includes(this.getUserId())) return true;
1096+
}
10961097

10971098
return false;
10981099
}).length > 0;
@@ -4660,7 +4661,7 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
46604661
room?.addLocalEchoReceipt(this.credentials.userId, rpEvent, ReceiptType.ReadPrivate);
46614662
}
46624663

4663-
return this.setRoomReadMarkersHttpRequest(roomId, rmEventId, rrEventId, rpEventId);
4664+
return await this.setRoomReadMarkersHttpRequest(roomId, rmEventId, rrEventId, rpEventId);
46644665
}
46654666

46664667
/**
@@ -7500,7 +7501,7 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
75007501
* don't want other users to see the read receipts. This is experimental. Optional.
75017502
* @return {Promise} Resolves: the empty object, {}.
75027503
*/
7503-
public setRoomReadMarkersHttpRequest(
7504+
public async setRoomReadMarkersHttpRequest(
75047505
roomId: string,
75057506
rmEventId: string,
75067507
rrEventId: string,
@@ -7513,9 +7514,13 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
75137514
const content = {
75147515
[ReceiptType.FullyRead]: rmEventId,
75157516
[ReceiptType.Read]: rrEventId,
7516-
[ReceiptType.ReadPrivate]: rpEventId,
75177517
};
75187518

7519+
const privateField = await utils.getPrivateReadReceiptField(this);
7520+
if (privateField) {
7521+
content[privateField] = rpEventId;
7522+
}
7523+
75197524
return this.http.authedRequest(undefined, Method.Post, path, undefined, content);
75207525
}
75217526

‎src/models/room.ts‎

Lines changed: 55 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2514,7 +2514,7 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
25142514
*/
25152515
public getUsersReadUpTo(event: MatrixEvent): string[] {
25162516
return this.getReceiptsForEvent(event).filter(function(receipt) {
2517-
return [ReceiptType.Read, ReceiptType.ReadPrivate].includes(receipt.type);
2517+
return utils.isSupportedReceiptType(receipt.type);
25182518
}).map(function(receipt) {
25192519
return receipt.userId;
25202520
});
@@ -2548,25 +2548,64 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
25482548
* @return {String} ID of the latest event that the given user has read, or null.
25492549
*/
25502550
public getEventReadUpTo(userId: string, ignoreSynthesized = false): string | null {
2551+
// XXX: This is very very ugly and I hope I won't have to ever add a new
2552+
// receipt type here again. IMHO this should be done by the server in
2553+
// some more intelligent manner or the client should just use timestamps
2554+
25512555
const timelineSet = this.getUnfilteredTimelineSet();
2552-
const publicReadReceipt = this.getReadReceiptForUserId(userId, ignoreSynthesized, ReceiptType.Read);
2553-
const privateReadReceipt = this.getReadReceiptForUserId(userId, ignoreSynthesized, ReceiptType.ReadPrivate);
2556+
const publicReadReceipt = this.getReadReceiptForUserId(
2557+
userId,
2558+
ignoreSynthesized,
2559+
ReceiptType.Read,
2560+
);
2561+
const privateReadReceipt = this.getReadReceiptForUserId(
2562+
userId,
2563+
ignoreSynthesized,
2564+
ReceiptType.ReadPrivate,
2565+
);
2566+
const unstablePrivateReadReceipt = this.getReadReceiptForUserId(
2567+
userId,
2568+
ignoreSynthesized,
2569+
ReceiptType.UnstableReadPrivate,
2570+
);
25542571

2555-
// If we have both, compare them
2556-
let comparison: number | undefined;
2557-
if (publicReadReceipt?.eventId && privateReadReceipt?.eventId) {
2558-
comparison = timelineSet.compareEventOrdering(publicReadReceipt?.eventId, privateReadReceipt?.eventId);
2572+
// If we have all, compare them
2573+
if (publicReadReceipt?.eventId && privateReadReceipt?.eventId && unstablePrivateReadReceipt?.eventId) {
2574+
const comparison1 = timelineSet.compareEventOrdering(
2575+
publicReadReceipt.eventId,
2576+
privateReadReceipt.eventId,
2577+
);
2578+
const comparison2 = timelineSet.compareEventOrdering(
2579+
publicReadReceipt.eventId,
2580+
unstablePrivateReadReceipt.eventId,
2581+
);
2582+
const comparison3 = timelineSet.compareEventOrdering(
2583+
privateReadReceipt.eventId,
2584+
unstablePrivateReadReceipt.eventId,
2585+
);
2586+
if (comparison1 && comparison2 && comparison3) {
2587+
return (comparison1 > 0)
2588+
? ((comparison2 > 0) ? publicReadReceipt.eventId : unstablePrivateReadReceipt.eventId)
2589+
: ((comparison3 > 0) ? privateReadReceipt.eventId : unstablePrivateReadReceipt.eventId);
2590+
}
25592591
}
25602592

2561-
// If we didn't get a comparison try to compare the ts of the receipts
2562-
if (!comparison) comparison = publicReadReceipt?.data?.ts - privateReadReceipt?.data?.ts;
2563-
2564-
// The public receipt is more likely to drift out of date so the private
2565-
// one has precedence
2566-
if (!comparison) return privateReadReceipt?.eventId ?? publicReadReceipt?.eventId ?? null;
2567-
2568-
// If public read receipt is older, return the private one
2569-
return (comparison < 0) ? privateReadReceipt?.eventId : publicReadReceipt?.eventId;
2593+
let latest = privateReadReceipt;
2594+
[unstablePrivateReadReceipt, publicReadReceipt].forEach((receipt) => {
2595+
if (receipt?.data?.ts > latest?.data?.ts) {
2596+
latest = receipt;
2597+
}
2598+
});
2599+
if (latest?.eventId) return latest?.eventId;
2600+
2601+
// The more less likely it is for a read receipt to drift out of date
2602+
// the bigger is its precedence
2603+
return (
2604+
privateReadReceipt?.eventId ??
2605+
unstablePrivateReadReceipt?.eventId ??
2606+
publicReadReceipt?.eventId ??
2607+
null
2608+
);
25702609
}
25712610

25722611
/**

‎src/sync-accumulator.ts‎

Lines changed: 8 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ limitations under the License.
2020
*/
2121

2222
import { logger } from './logger';
23-
import { deepCopy } from "./utils";
23+
import { deepCopy, isSupportedReceiptType } from "./utils";
2424
import { IContent, IUnsigned } from "./models/event";
2525
import { IRoomSummary } from "./models/room-summary";
2626
import { EventType } from "./@types/event";
@@ -417,31 +417,18 @@ export class SyncAccumulator {
417417
// of a hassle to work with. We'll inflate this back out when
418418
// getJSON() is called.
419419
Object.keys(e.content).forEach((eventId) => {
420-
if (!e.content[eventId][ReceiptType.Read] && !e.content[eventId][ReceiptType.ReadPrivate]) {
421-
return;
422-
}
423-
const read = e.content[eventId][ReceiptType.Read];
424-
if (read) {
425-
Object.keys(read).forEach((userId) => {
426-
// clobber on user ID
427-
currentData._readReceipts[userId] = {
428-
data: e.content[eventId][ReceiptType.Read][userId],
429-
type: ReceiptType.Read,
430-
eventId: eventId,
431-
};
432-
});
433-
}
434-
const readPrivate = e.content[eventId][ReceiptType.ReadPrivate];
435-
if (readPrivate) {
436-
Object.keys(readPrivate).forEach((userId) => {
420+
Object.entries(e.content[eventId]).forEach(([key, value]) => {
421+
if (!isSupportedReceiptType(key)) return;
422+
423+
Object.keys(value).forEach((userId) => {
437424
// clobber on user ID
438425
currentData._readReceipts[userId] = {
439-
data: e.content[eventId][ReceiptType.ReadPrivate][userId],
440-
type: ReceiptType.ReadPrivate,
426+
data: e.content[eventId][key][userId],
427+
type: key as ReceiptType,
441428
eventId: eventId,
442429
};
443430
});
444-
}
431+
});
445432
});
446433
});
447434
}

0 commit comments

Comments
 (0)