Skip to content

Commit 9a891cf

Browse files
MartinCupelaMartinCupela
committed
fix: prevent overriding read data on various message delivery events
1 parent 468bf90 commit 9a891cf

File tree

4 files changed

+181
-26
lines changed

4 files changed

+181
-26
lines changed

‎src/channel.ts‎

Lines changed: 11 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1894,15 +1894,14 @@ export class Channel {
18941894
break;
18951895
case 'message.read':
18961896
if (event.user?.id && event.created_at) {
1897+
const previousReadState = channelState.read[event.user.id];
18971898
channelState.read[event.user.id] = {
1899+
// in case we already have delivery information
1900+
...previousReadState,
18981901
last_read: new Date(event.created_at),
18991902
last_read_message_id: event.last_read_message_id,
19001903
user: event.user,
19011904
unread_messages: 0,
1902-
last_delivered_at: event.last_delivered_at
1903-
? new Date(event.last_delivered_at)
1904-
: undefined,
1905-
last_delivered_message_id: event.last_delivered_message_id,
19061905
};
19071906
this.ownMessageReceiptsTracker.onMessageRead({
19081907
user: event.user,
@@ -1922,15 +1921,14 @@ export class Channel {
19221921
case 'message.delivered':
19231922
// todo: update also on thread
19241923
if (event.user?.id && event.created_at) {
1924+
const previousReadState = channelState.read[event.user.id];
19251925
channelState.read[event.user.id] = {
1926-
last_read: new Date(event.created_at),
1927-
last_read_message_id: event.last_read_message_id,
1928-
user: event.user,
1929-
unread_messages: event.unread_messages ?? 0,
1926+
...previousReadState,
19301927
last_delivered_at: event.last_delivered_at
19311928
? new Date(event.last_delivered_at)
19321929
: undefined,
19331930
last_delivered_message_id: event.last_delivered_message_id,
1931+
user: event.user,
19341932
};
19351933

19361934
this.ownMessageReceiptsTracker.onMessageDelivered({
@@ -1998,17 +1996,12 @@ export class Channel {
19981996
if (event.user?.id) {
19991997
for (const userId in channelState.read) {
20001998
if (userId === event.user.id) {
2001-
const currentState = channelState.read[event.user.id];
20021999
channelState.read[event.user.id] = {
20032000
last_read: new Date(event.created_at as string),
20042001
user: event.user,
20052002
unread_messages: 0,
2006-
last_delivered_at: event.last_delivered_at
2007-
? new Date(event.last_delivered_at)
2008-
: currentState.last_delivered_at,
2009-
last_delivered_message_id:
2010-
event.last_delivered_message_id ??
2011-
currentState.last_delivered_message_id,
2003+
last_delivered_at: new Date(event.created_at as string),
2004+
last_delivered_message_id: event.message.id,
20122005
};
20132006
} else {
20142007
channelState.read[userId].unread_messages += 1;
@@ -2124,17 +2117,15 @@ export class Channel {
21242117
if (!ownMessage || !event.user) break;
21252118

21262119
const unreadCount = event.unread_messages ?? 0;
2127-
2120+
const currentState = channelState.read[event.user.id];
21282121
channelState.read[event.user.id] = {
2122+
// keep the message delivery info
2123+
...currentState,
21292124
first_unread_message_id: event.first_unread_message_id,
21302125
last_read: new Date(event.last_read_at as string),
21312126
last_read_message_id: event.last_read_message_id,
21322127
user: event.user,
21332128
unread_messages: unreadCount,
2134-
last_delivered_at: event.last_delivered_at
2135-
? new Date(event.last_delivered_at)
2136-
: undefined,
2137-
last_delivered_message_id: event.last_delivered_message_id,
21382129
};
21392130

21402131
channelState.unreadCount = unreadCount;

‎test/unit/OwnMessageReceiptsTracker.test.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ describe('OwnMessageDeliveryReadTracker', () => {
3939
let tracker: OwnMessageReceiptsTracker;
4040

4141
beforeEach(() => {
42-
tracker = new OwnMessageReceiptsTracker({ locateMessage: makeLocator(), ownUserId });
42+
tracker = new OwnMessageReceiptsTracker({ locateMessage: makeLocator() });
4343
});
4444

4545
describe('ingestInitial', () => {
@@ -129,7 +129,7 @@ describe('OwnMessageDeliveryReadTracker', () => {
129129
// re-init with a locator that knows only m1..m3 (m4 is unknown)
130130
const locator = (ts?: number) =>
131131
ts && ts <= 3000 ? { timestampMs: ts, msgId: byTs.get(ts)!.id } : null;
132-
tracker = new OwnMessageReceiptsTracker({ locateMessage: locator, ownUserId });
132+
tracker = new OwnMessageReceiptsTracker({ locateMessage: locator });
133133

134134
const dave = U('dave');
135135
tracker.onMessageRead({ user: dave, readAt: iso(4000) }); // unknown -> ignored
@@ -144,7 +144,7 @@ describe('OwnMessageDeliveryReadTracker', () => {
144144

145145
it('prevents search for message if last read message id is provided', () => {
146146
const locator = vi.fn().mockImplementation(() => {});
147-
tracker = new OwnMessageReceiptsTracker({ locateMessage: locator, ownUserId });
147+
tracker = new OwnMessageReceiptsTracker({ locateMessage: locator });
148148
const user = U('frank');
149149
tracker.onMessageRead({ user, readAt: iso(3000), lastReadMessageId: 'X' }); // unknown -> ignored
150150
expect(locator).not.toHaveBeenCalled();
@@ -203,7 +203,7 @@ describe('OwnMessageDeliveryReadTracker', () => {
203203
it('ignores delivered events with unknown timestamps (locator returns null)', () => {
204204
const locator = (t?: number) =>
205205
t && t <= 2000 ? { timestampMs: t, msgId: byTs.get(t)!.id } : null;
206-
tracker = new OwnMessageReceiptsTracker({ locateMessage: locator, ownUserId });
206+
tracker = new OwnMessageReceiptsTracker({ locateMessage: locator });
207207

208208
const frank = U('frank');
209209
tracker.onMessageDelivered({ user: frank, deliveredAt: iso(3000) }); // unknown -> ignored
@@ -216,7 +216,7 @@ describe('OwnMessageDeliveryReadTracker', () => {
216216

217217
it('prevents search for message if last read message id is provided', () => {
218218
const locator = vi.fn().mockImplementation(() => {});
219-
tracker = new OwnMessageReceiptsTracker({ locateMessage: locator, ownUserId });
219+
tracker = new OwnMessageReceiptsTracker({ locateMessage: locator });
220220
const user = U('frank');
221221
tracker.onMessageDelivered({
222222
user,

‎test/unit/channel.test.js‎

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -355,6 +355,8 @@ describe('Channel _handleChannelEvent', function () {
355355
expect(channel.state.unreadCount).to.be.equal(30);
356356
});
357357

358+
it('does not override the delivery information in the read status', () => {});
359+
358360
it('message.truncate removes all messages if "truncated_at" is "now"', function () {
359361
const messages = [
360362
{ created_at: '2021-01-01T00:01:00' },
@@ -652,6 +654,8 @@ describe('Channel _handleChannelEvent', function () {
652654
last_read_message_id: '6',
653655
user,
654656
unread_messages: initialCountUnread,
657+
last_delivered_at: new Date(1000).toISOString(),
658+
last_delivered_message_id: 'delivered-msg-id',
655659
};
656660
notificationMarkUnreadEvent = {
657661
type: 'notification.mark_unread',
@@ -690,6 +694,12 @@ describe('Channel _handleChannelEvent', function () {
690694
expect(channel.state.read[user.id].unread_messages).to.be.equal(
691695
event.unread_messages,
692696
);
697+
expect(channel.state.read[user.id].last_delivered_at).toBe(
698+
initialReadState.last_delivered_at,
699+
);
700+
expect(channel.state.read[user.id].last_delivered_message_id).toBe(
701+
initialReadState.last_delivered_message_id,
702+
);
693703
});
694704

695705
it('should not update channel read state produced for another user or user is missing', () => {
@@ -718,6 +728,160 @@ describe('Channel _handleChannelEvent', function () {
718728
});
719729
});
720730

731+
describe('message.read', () => {
732+
let initialCountUnread;
733+
let initialReadState;
734+
let messageReadEvent;
735+
736+
beforeEach(() => {
737+
initialCountUnread = 100;
738+
initialReadState = {
739+
last_read: new Date(1500).toISOString(),
740+
last_read_message_id: '6',
741+
user,
742+
unread_messages: initialCountUnread,
743+
last_delivered_at: new Date(1000).toISOString(),
744+
last_delivered_message_id: 'delivered-msg-id',
745+
};
746+
messageReadEvent = {
747+
type: 'message.read',
748+
created_at: new Date(2000).toISOString(),
749+
cid: channel.cid,
750+
channel_member_count: 100,
751+
channel_type: channel.type,
752+
channel_id: channel.id,
753+
user,
754+
last_read_message_id: '6b1006ad-7a6d-49d1-82d9-5ee5e8167e49',
755+
};
756+
});
757+
758+
it('should update channel read state produced for current user', () => {
759+
channel.state.unreadCount = initialCountUnread;
760+
channel.state.read[user.id] = initialReadState;
761+
const event = messageReadEvent;
762+
763+
channel._handleChannelEvent(event);
764+
765+
expect(channel.state.unreadCount).toBe(0);
766+
expect(new Date(channel.state.read[user.id].last_read).getTime()).toBe(
767+
new Date(messageReadEvent.created_at).getTime(),
768+
);
769+
expect(channel.state.read[user.id].last_read_message_id).toBe(
770+
event.last_read_message_id,
771+
);
772+
expect(channel.state.read[user.id].unread_messages).toBe(0);
773+
expect(channel.state.read[user.id].last_delivered_at).toBe(
774+
initialReadState.last_delivered_at,
775+
);
776+
expect(channel.state.read[user.id].last_delivered_message_id).toBe(
777+
initialReadState.last_delivered_message_id,
778+
);
779+
});
780+
781+
it('should update channel read state produced for another user', () => {
782+
const anotherUser = { id: 'another-user' };
783+
channel.state.unreadCount = initialCountUnread;
784+
channel.state.read[anotherUser.id] = initialReadState;
785+
const event = { ...messageReadEvent, user: anotherUser };
786+
787+
channel._handleChannelEvent(event);
788+
789+
expect(channel.state.unreadCount).toBe(initialCountUnread);
790+
expect(new Date(channel.state.read[anotherUser.id].last_read).getTime()).toBe(
791+
new Date(messageReadEvent.created_at).getTime(),
792+
);
793+
expect(channel.state.read[anotherUser.id].last_read_message_id).toBe(
794+
event.last_read_message_id,
795+
);
796+
expect(channel.state.read[anotherUser.id].unread_messages).toBe(0);
797+
expect(channel.state.read[anotherUser.id].last_delivered_at).toBe(
798+
initialReadState.last_delivered_at,
799+
);
800+
expect(channel.state.read[anotherUser.id].last_delivered_message_id).toBe(
801+
initialReadState.last_delivered_message_id,
802+
);
803+
});
804+
});
805+
806+
describe('message.delivered', () => {
807+
let initialCountUnread;
808+
let initialReadState;
809+
let messageDeliveredEvent;
810+
811+
beforeEach(() => {
812+
initialCountUnread = 100;
813+
initialReadState = {
814+
last_read: new Date(1500).toISOString(),
815+
last_read_message_id: '6',
816+
user,
817+
unread_messages: initialCountUnread,
818+
last_delivered_at: new Date(1000).toISOString(),
819+
last_delivered_message_id: 'delivered-msg-id',
820+
};
821+
messageDeliveredEvent = {
822+
type: 'message.delivered',
823+
created_at: new Date(2000).toISOString(),
824+
cid: channel.cid,
825+
channel_member_count: 100,
826+
channel_type: channel.type,
827+
channel_id: channel.id,
828+
user,
829+
last_delivered_message_id: 'fd403be5-9207-48db-8bd7-13bd65ffbea6',
830+
last_delivered_at: new Date(2000).toISOString(),
831+
};
832+
});
833+
834+
it('should update channel read state produced for current user', () => {
835+
channel.state.unreadCount = initialCountUnread;
836+
channel.state.read[user.id] = initialReadState;
837+
838+
channel._handleChannelEvent(messageDeliveredEvent);
839+
840+
expect(channel.state.unreadCount).toBe(initialReadState.unread_messages);
841+
expect(new Date(channel.state.read[user.id].last_read).getTime()).toBe(
842+
new Date(initialReadState.last_read).getTime(),
843+
);
844+
expect(channel.state.read[user.id].last_read_message_id).toBe(
845+
initialReadState.last_read_message_id,
846+
);
847+
expect(channel.state.read[user.id].unread_messages).toBe(
848+
initialReadState.unread_messages,
849+
);
850+
expect(new Date(channel.state.read[user.id].last_delivered_at).getTime()).toBe(
851+
new Date(messageDeliveredEvent.last_delivered_at).getTime(),
852+
);
853+
expect(channel.state.read[user.id].last_delivered_message_id).toBe(
854+
messageDeliveredEvent.last_delivered_message_id,
855+
);
856+
});
857+
858+
it('should update channel read state produced for another user', () => {
859+
const anotherUser = { id: 'another-user' };
860+
channel.state.unreadCount = initialCountUnread;
861+
channel.state.read[anotherUser.id] = initialReadState;
862+
const event = { ...messageDeliveredEvent, user: anotherUser };
863+
864+
channel._handleChannelEvent(event);
865+
866+
expect(channel.state.unreadCount).toBe(initialCountUnread);
867+
expect(new Date(channel.state.read[anotherUser.id].last_read).getTime()).toBe(
868+
new Date(initialReadState.last_read).getTime(),
869+
);
870+
expect(channel.state.read[anotherUser.id].last_read_message_id).toBe(
871+
initialReadState.last_read_message_id,
872+
);
873+
expect(channel.state.read[anotherUser.id].unread_messages).toBe(
874+
initialReadState.unread_messages,
875+
);
876+
expect(
877+
new Date(channel.state.read[anotherUser.id].last_delivered_at).getTime(),
878+
).toBe(new Date(event.last_delivered_at).getTime());
879+
expect(channel.state.read[anotherUser.id].last_delivered_message_id).toBe(
880+
event.last_delivered_message_id,
881+
);
882+
});
883+
});
884+
721885
it('should include unread_messages for message events from another user', () => {
722886
channel.state.read['id'] = {
723887
unread_messages: 2,

‎test/unit/utils.test.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2718,7 +2718,7 @@ describe('messageSetPagination', () => {
27182718
});
27192719
});
27202720

2721-
describe('', () => {
2721+
describe('binarySearchByDateEqualOrNearestGreater', () => {
27222722
const messages = [
27232723
{ created_at: '2024-08-05T08:55:00.199808Z', id: '0' },
27242724
{ created_at: '2024-08-05T08:55:01.199808Z', id: '1' },

0 commit comments

Comments
 (0)