From 3280d51afd955a611d15b65166efa959c18a6b89 Mon Sep 17 00:00:00 2001 From: DomW Date: Wed, 4 Dec 2024 13:48:40 +0000 Subject: [PATCH] refactor(matrix-client/notification-saga): remove outdated notification processing (#2484) * feat(notifications): use room unread count to process notifications * refactor(matrix-client/notification-saga): remove outdated notification processing * refactor: remove unused utils --- .../notification-item/utils.test.ts | 62 --------- .../notification-item/utils.ts | 18 --- src/lib/chat/index.ts | 12 -- src/lib/chat/matrix-client.ts | 121 ------------------ src/lib/chat/matrix/chat-message.ts | 71 ---------- src/store/notifications/index.ts | 75 +---------- src/store/notifications/saga.test.ts | 77 +---------- src/store/notifications/saga.ts | 53 +------- src/store/reducer.ts | 2 - 9 files changed, 4 insertions(+), 487 deletions(-) delete mode 100644 src/components/notifications-feed/notification-item/utils.test.ts delete mode 100644 src/components/notifications-feed/notification-item/utils.ts diff --git a/src/components/notifications-feed/notification-item/utils.test.ts b/src/components/notifications-feed/notification-item/utils.test.ts deleted file mode 100644 index a0d3d8a20..000000000 --- a/src/components/notifications-feed/notification-item/utils.test.ts +++ /dev/null @@ -1,62 +0,0 @@ -import { getNotificationContent } from './utils'; - -describe('Notification Feed Utils', () => { - describe(getNotificationContent, () => { - it('returns correct message for reply notification', () => { - const notification = { - type: 'reply', - sender: { firstName: 'John' }, - } as any; - - expect(getNotificationContent(notification)).toBe('John replied to your message'); - }); - - it('returns correct message for direct message notification', () => { - const notification = { - type: 'direct_message', - sender: { firstName: 'Jane' }, - } as any; - - expect(getNotificationContent(notification)).toBe('Jane sent you a direct message'); - }); - - it('returns correct message for mention notification', () => { - const notification = { - type: 'mention', - sender: { firstName: 'Bob' }, - } as any; - - expect(getNotificationContent(notification)).toBe('Bob mentioned you in conversation'); - }); - - it('returns correct message for reaction notification', () => { - const notification = { - type: 'reaction', - sender: { firstName: 'Charlie' }, - content: { - reactionKey: '👍', - }, - } as any; - - expect(getNotificationContent(notification)).toBe('Charlie reacted to your message with 👍'); - }); - - it('uses "Someone" when sender firstName is not available', () => { - const notification = { - type: 'direct_message', - sender: {}, - } as any; - - expect(getNotificationContent(notification)).toBe('Someone sent you a direct message'); - }); - - it('returns default message for unknown notification type', () => { - const notification = { - type: 'unknown_type', - sender: { firstName: 'Dave' }, - } as any; - - expect(getNotificationContent(notification)).toBe('Dave interacted with you'); - }); - }); -}); diff --git a/src/components/notifications-feed/notification-item/utils.ts b/src/components/notifications-feed/notification-item/utils.ts deleted file mode 100644 index d4467bf1f..000000000 --- a/src/components/notifications-feed/notification-item/utils.ts +++ /dev/null @@ -1,18 +0,0 @@ -import { Notification } from '../../../store/notifications'; - -export function getNotificationContent(notification: Notification): string { - const senderName = notification.sender?.firstName || 'Someone'; - - switch (notification.type) { - case 'reply': - return `${senderName} replied to your message`; - case 'direct_message': - return `${senderName} sent you a direct message`; - case 'mention': - return `${senderName} mentioned you in conversation`; - case 'reaction': - return `${senderName} reacted to your message with ${notification?.content?.reactionKey}`; - default: - return `${senderName} interacted with you`; - } -} diff --git a/src/lib/chat/index.ts b/src/lib/chat/index.ts index 4b59c11d9..a3de00e0a 100644 --- a/src/lib/chat/index.ts +++ b/src/lib/chat/index.ts @@ -424,15 +424,3 @@ export function getProfileInfo(userId: string): Promise<{ }> { return chat.get().matrix.getProfileInfo(userId); } - -export function getNotifications(): Promise { - return chat.get().matrix.getNotifications(); -} - -export async function getNotificationReadStatus() { - return await chat.get().matrix.getNotificationReadStatus(); -} - -export async function setNotificationReadStatus(roomId: string) { - return await chat.get().matrix.setNotificationReadStatus(roomId); -} diff --git a/src/lib/chat/matrix-client.ts b/src/lib/chat/matrix-client.ts index 2e9a3fd92..e9fd04339 100644 --- a/src/lib/chat/matrix-client.ts +++ b/src/lib/chat/matrix-client.ts @@ -23,7 +23,6 @@ import { import { RealtimeChatEvents, IChatClient } from './'; import { mapEventToAdminMessage, - mapEventToNotification, mapEventToPostMessage, mapMatrixMessage, mapToLiveRoomEvent, @@ -69,8 +68,6 @@ export class MatrixClient implements IChatClient { private initializationTimestamp: number; private secretStorageKey: string; - private readonly NOTIFICATION_READ_EVENT = 'org.zero.notifications.read'; - constructor(private sdk = { createClient }, private sessionStorage = new SessionStorage()) { this.addConnectionAwaiter(); } @@ -345,34 +342,6 @@ export class MatrixClient implements IChatClient { .then((response) => response?.body || []); } - async getNotificationReadStatus(): Promise> { - await this.waitForConnection(); - try { - const event = await this.matrix.getAccountData(this.NOTIFICATION_READ_EVENT); - return event?.getContent()?.readNotifications || {}; - } catch (error) { - console.error('Error getting notification read status:', error); - return {}; - } - } - - async setNotificationReadStatus(roomId: string): Promise { - await this.waitForConnection(); - try { - const currentContent = await this.getNotificationReadStatus(); - const updatedContent = { - readNotifications: { - ...currentContent, - [roomId]: Date.now(), - }, - }; - - await this.matrix.setAccountData(this.NOTIFICATION_READ_EVENT, updatedContent); - } catch (error) { - console.error('Error setting notification read status:', error); - } - } - async searchMentionableUsersForChannel(channelId: string, search: string, channelMembers: UserModel[]) { const searchResults = await getFilteredMembersForAutoComplete(channelMembers, search); return searchResults.map((u) => ({ @@ -636,50 +605,6 @@ export class MatrixClient implements IChatClient { return result; } - async getNotifications(): Promise { - await this.waitForConnection(); - const rooms = await this.getRoomsUserIsIn(); - const CUTOFF_DAYS = 30; - const CUTOFF_TIMESTAMP = Date.now() - CUTOFF_DAYS * 24 * 60 * 60 * 1000; - let allNotifications = []; - - let readStatus = {}; - if (featureFlags.enableNotificationsReadStatus) { - readStatus = await this.getNotificationReadStatus(); - } - - const roomNotifications = await Promise.all( - rooms.map(async (room) => { - const timeline = room.getLiveTimeline(); - const events = timeline.getEvents(); - - const mostRecentEvent = events[events.length - 1]; - const oldestEvent = events[0]; - - if (!oldestEvent || !mostRecentEvent || (oldestEvent.getTs() > CUTOFF_TIMESTAMP && events.length < 50)) { - await this.matrix.paginateEventTimeline(timeline, { - backwards: true, - limit: 50, - }); - } - - if (room.hasEncryptionStateEvent() && events.some((e) => e.getTs() > CUTOFF_TIMESTAMP)) { - await room.decryptAllEvents(); - } - - const filteredEvents = timeline - .getEvents() - .filter((event) => event.getTs() > CUTOFF_TIMESTAMP) - .map((event) => event.getEffectiveEvent()); - - return this.getNotificationsFromRoom(filteredEvents, readStatus); - }) - ); - - allNotifications = roomNotifications.flat(); - return allNotifications.sort((a, b) => b.createdAt - a.createdAt); - } - async getMessageByRoomId(channelId: string, messageId: string) { await this.waitForConnection(); const newMessage = await this.matrix.fetchRoomEvent(channelId, messageId); @@ -1719,52 +1644,6 @@ export class MatrixClient implements IChatClient { return event.type === CustomEventType.ROOM_POST; } - private async getNotificationsFromRoom(events, readStatus = {}): Promise { - const currentUserId = this.matrix.getUserId(); - const currentUserUuid = currentUserId.split(':')[0].substring(1); - - const filteredEvents = events.filter((event) => { - const relatesTo = event.content && event.content['m.relates_to']; - const isDM = this.matrix.getRoom(event.room_id)?.getMembers().length === 2; - const isFromCurrentUser = event.sender === currentUserId; - - // Don't process notifications from the current user - if (isFromCurrentUser) return false; - - if ( - event.type === 'm.room.message' && - event.content?.body?.includes('@[') && - event.content?.body?.includes('](user:') && - event.content.body.includes(currentUserUuid) - ) { - return true; - } - - if (relatesTo?.['m.in_reply_to']) { - const originalEvent = events.find((e) => e.event_id === relatesTo['m.in_reply_to'].event_id); - return originalEvent?.sender === currentUserId; - } - - if (event.type === MatrixConstants.REACTION && !event?.unsigned?.redacted_because) { - const targetEvent = events.find((e) => e.event_id === relatesTo.event_id); - return targetEvent?.sender === currentUserId; - } - - return isDM && event.type === 'm.room.message'; - }); - - const notifications = await Promise.all( - filteredEvents.map((event) => { - const roomReadTimestamp = readStatus[event.room_id] || 0; - const isRead = featureFlags.enableNotificationsReadStatus ? event.origin_server_ts <= roomReadTimestamp : false; - - return mapEventToNotification(event, isRead); - }) - ); - - return notifications.filter((notification) => notification !== null); - } - // Performance improvement: Fetches only the latest user message to avoid processing large image files and other attachments // that are not needed for the initial loading of the conversation list private async getUpToLatestUserMessageFromRoom(room: Room): Promise { diff --git a/src/lib/chat/matrix/chat-message.ts b/src/lib/chat/matrix/chat-message.ts index 8ce5b12b8..67ac65525 100644 --- a/src/lib/chat/matrix/chat-message.ts +++ b/src/lib/chat/matrix/chat-message.ts @@ -142,77 +142,6 @@ export async function mapEventToPostMessage(matrixMessage, sdkMatrixClient: SDKM }; } -export async function mapEventToNotification(event, isRead: boolean = false) { - const { event_id, room_id, origin_server_ts, sender, content, type } = event; - - const baseNotification = { - id: event_id, - roomId: room_id, - createdAt: origin_server_ts, - isRead, - sender: { - userId: sender, - }, - }; - - if (type === 'm.room.message' && content?.body?.includes('@[')) { - return { - ...baseNotification, - type: 'mention', - content: { - body: content.body, - mentions: extractMentionsFromBody(content.body), - notificationType: 'mention', - }, - }; - } - - if (type === MatrixConstants.REACTION && !event?.unsigned?.redacted_because) { - const isMeowReaction = content['m.relates_to'].key.startsWith('MEOW_'); - if (isMeowReaction) return null; - - return { - ...baseNotification, - type: 'reaction', - content: { - reactionKey: content['m.relates_to'].key, - targetEventId: content['m.relates_to'].event_id, - }, - }; - } - - if (content['m.relates_to']?.['m.in_reply_to']) { - return { - ...baseNotification, - type: 'reply', - content: { - body: content.body, - replyToEventId: content['m.relates_to']['m.in_reply_to'].event_id, - notificationType: 'reply', - }, - }; - } - - if (type === 'm.room.message') { - return { - ...baseNotification, - type: 'direct_message', - content: { - body: content.body, - notificationType: 'direct_message', - }, - }; - } - - return null; -} - -export function extractMentionsFromBody(body: string) { - const mentionRegex = /@\[.*?\]\(user:(.*?)\)/g; - const matches = [...body?.matchAll(mentionRegex)]; - return matches.map((match) => match[1]); -} - function getAdminDataFromEventType(type, content, sender, targetUserId, previousContent) { switch (type) { case EventType.RoomMember: diff --git a/src/store/notifications/index.ts b/src/store/notifications/index.ts index b4fcee6af..677b913be 100644 --- a/src/store/notifications/index.ts +++ b/src/store/notifications/index.ts @@ -1,80 +1,7 @@ -import { createAction, createSlice } from '@reduxjs/toolkit'; +import { createAction } from '@reduxjs/toolkit'; export enum SagaActionTypes { - FetchNotifications = 'notifications/saga/fetchNotifications', OpenNotificationConversation = 'notifications/saga/openNotificationConversation', - MarkNotificationsAsRead = 'notifications/saga/markNotificationsAsRead', } -export interface Notification { - id: string; - type: 'reply' | 'mention' | 'direct_message' | 'reaction'; - roomId: string; - createdAt: number; - isRead?: boolean; - sender?: { - userId: string; - firstName?: string; - profileImage?: string; - }; - content: { - body?: string; - replyToEventId?: string; - reactionKey?: string; - targetEventId?: string; - amount?: number; - }; -} - -export interface NotificationsState { - items: Notification[]; - loading: boolean; - error: string | null; -} - -export const initialState: NotificationsState = { - items: [], - loading: false, - error: null, -}; - -export const fetchNotifications = createAction(SagaActionTypes.FetchNotifications); export const openNotificationConversation = createAction(SagaActionTypes.OpenNotificationConversation); -export const markNotificationsAsRead = createAction(SagaActionTypes.MarkNotificationsAsRead); - -const slice = createSlice({ - name: 'notifications', - initialState, - reducers: { - setNotifications: (state, action) => { - state.items = action.payload; - }, - - setLoading: (state, action) => { - state.loading = action.payload; - }, - - setError: (state, action) => { - state.error = action.payload; - }, - - markAsRead: (state, action) => { - const roomId = action.payload; - const hasUnreadNotifications = state.items.some( - (notification) => notification.roomId === roomId && !notification.isRead - ); - - if (hasUnreadNotifications) { - state.items = state.items.map((notification) => { - if (notification.roomId === roomId) { - return { ...notification, isRead: true }; - } - return notification; - }); - } - }, - }, -}); - -export const { setNotifications, setLoading, setError, markAsRead } = slice.actions; -export const { reducer } = slice; diff --git a/src/store/notifications/saga.test.ts b/src/store/notifications/saga.test.ts index 252eb9b7b..c5e8b9bd2 100644 --- a/src/store/notifications/saga.test.ts +++ b/src/store/notifications/saga.test.ts @@ -1,86 +1,11 @@ import { expectSaga } from 'redux-saga-test-plan'; import { call } from 'redux-saga/effects'; -import { fetchNotifications, openNotificationConversation } from './saga'; -import { setNotifications, setLoading, setError } from '.'; -import { getNotifications } from '../../lib/chat'; +import { openNotificationConversation } from './saga'; import { getHistory } from '../../lib/browser'; import { openConversation } from '../channels/saga'; -import { mapNotificationSenders } from '../messages/utils.matrix'; describe('notifications saga', () => { - describe('fetchNotifications', () => { - it('fetches and sets notifications successfully when no existing notifications', async () => { - const mockNotifications = [ - { id: '1', content: 'notification 1', createdAt: Date.now() }, - { id: '2', content: 'notification 2', createdAt: Date.now() }, - ]; - - const mockNotificationsWithSenders = [ - { id: '1', content: 'notification 1', sender: { id: 'sender1' }, createdAt: Date.now() }, - { id: '2', content: 'notification 2', sender: { id: 'sender2' }, createdAt: Date.now() }, - ]; - - await expectSaga(fetchNotifications) - .withState({ - notifications: { - items: [], - }, - }) - .provide([ - [call(getNotifications), mockNotifications], - [call(mapNotificationSenders, mockNotifications), mockNotificationsWithSenders], - ]) - .put(setLoading(true)) - .call(getNotifications) - .call(mapNotificationSenders, mockNotifications) - .put(setNotifications(mockNotificationsWithSenders)) - .put(setLoading(false)) - .run(); - }); - - it('uses existing notifications if they are recent and sufficient', async () => { - const now = Date.now(); - const existingNotifications = Array.from({ length: 51 }, (_, i) => ({ - id: `${i}`, - content: `notification ${i}`, - createdAt: now - i * 1000, - })); - await expectSaga(fetchNotifications) - .withState({ - notifications: { - items: existingNotifications, - }, - }) - .provide([ - [call(getNotifications), existingNotifications], - ]) - .put(setLoading(true)) - .put(setLoading(false)) - .run(); - }); - - it('handles errors when fetching notifications', async () => { - const error = new Error('Failed to fetch notifications'); - const consoleSpy = jest.spyOn(console, 'error').mockImplementation(); - - await expectSaga(fetchNotifications) - .withState({ - notifications: { - items: [], - }, - }) - .provide([[call(getNotifications), Promise.reject(error)]]) - .put(setLoading(true)) - .put(setError(error.message)) - .put(setLoading(false)) - .run(); - - expect(consoleSpy).toHaveBeenCalledWith('Error fetching notifications:', error); - consoleSpy.mockRestore(); - }); - }); - describe('openNotificationConversation', () => { it('opens conversation and navigates to the correct URL', async () => { const roomId = 'room-123'; diff --git a/src/store/notifications/saga.ts b/src/store/notifications/saga.ts index ce4dcc863..86460f70c 100644 --- a/src/store/notifications/saga.ts +++ b/src/store/notifications/saga.ts @@ -1,54 +1,7 @@ -import { takeLatest, call, put, select } from 'redux-saga/effects'; -import { SagaActionTypes, setNotifications, setLoading, setError, markAsRead } from '.'; +import { takeLatest, call } from 'redux-saga/effects'; +import { SagaActionTypes } from '.'; import { openConversation } from '../channels/saga'; -import { getNotifications, setNotificationReadStatus } from '../../lib/chat'; import { getHistory } from '../../lib/browser'; -import { mapNotificationSenders } from '../messages/utils.matrix'; - -function* markNotificationsAsReadSaga(action) { - const roomId = action.payload; - try { - yield call(setNotificationReadStatus, roomId); - yield put(markAsRead(roomId)); - } catch (error) { - console.error('Error marking notifications as read:', error); - } -} -export function* fetchNotifications() { - try { - yield put(setLoading(true)); - - const NOTIFICATION_LIMIT = 50; - const CUTOFF_DAYS = 30; - const CUTOFF_TIMESTAMP = Date.now() - CUTOFF_DAYS * 24 * 60 * 60 * 1000; - - // First check existing notifications in state - const existingNotifications = yield select((state) => state.notifications.items); - if (existingNotifications?.length > 0) { - const recentNotifications = existingNotifications.filter((n) => n.createdAt > CUTOFF_TIMESTAMP); - if (recentNotifications.length >= NOTIFICATION_LIMIT) { - yield put(setNotifications(recentNotifications.slice(0, NOTIFICATION_LIMIT))); - return; - } - } - - // If we don't have enough recent notifications, fetch new ones - const notifications = yield call(getNotifications); - const notificationsWithSenders = yield call(mapNotificationSenders, notifications); - - const sortedNotifications = notificationsWithSenders - .filter((n) => n.createdAt > CUTOFF_TIMESTAMP) - .sort((a, b) => b.createdAt - a.createdAt) - .slice(0, NOTIFICATION_LIMIT); - - yield put(setNotifications(sortedNotifications)); - } catch (error: any) { - console.error('Error fetching notifications:', error); - yield put(setError(error.message)); - } finally { - yield put(setLoading(false)); - } -} export function* openNotificationConversation(action) { const roomId = action.payload; @@ -69,7 +22,5 @@ export function* openNotificationConversation(action) { } export function* saga() { - yield takeLatest(SagaActionTypes.FetchNotifications, fetchNotifications); yield takeLatest(SagaActionTypes.OpenNotificationConversation, openNotificationConversation); - yield takeLatest(SagaActionTypes.MarkNotificationsAsRead, markNotificationsAsReadSaga); } diff --git a/src/store/reducer.ts b/src/store/reducer.ts index 9dfbe25a6..9f56b4435 100644 --- a/src/store/reducer.ts +++ b/src/store/reducer.ts @@ -23,7 +23,6 @@ import { reducer as userProfile } from './user-profile'; import { reducer as background } from './background'; import { reducer as accountManagement } from './account-management'; import { reducer as posts } from './posts'; -import { reducer as notifications } from './notifications'; export const rootReducer = combineReducers({ pageload, @@ -49,7 +48,6 @@ export const rootReducer = combineReducers({ background, accountManagement, posts, - notifications, }); export type RootState = ReturnType;