From 67d6a893340d0a9c20c1dd7d3e76374dce013ff1 Mon Sep 17 00:00:00 2001 From: Cypher Pepe <125112044+cypherpepe@users.noreply.github.com> Date: Tue, 24 Sep 2024 17:27:27 +0300 Subject: [PATCH 1/2] Update notifications.ts The main improvement is the removal of repeated logic for filtering and updating notifications. This is achieved by centralizing it in the 'getUpdatedTopicState' function, making the reducer more readable and reducing potential errors from code duplication. --- src/reducers/notifications.ts | 80 ++++++++++++++++++----------------- 1 file changed, 42 insertions(+), 38 deletions(-) diff --git a/src/reducers/notifications.ts b/src/reducers/notifications.ts index 1350a3d5..234c1e42 100644 --- a/src/reducers/notifications.ts +++ b/src/reducers/notifications.ts @@ -28,80 +28,84 @@ export type NotificationsActions = topic: string } -// Opted for a reducer since the state changes are complex enough to warrant -// changes to a set and an array. Having all that inside the hooks would -// cause too many updates to the hooks, causing unnecessary rerenders. -export const notificationsReducer = ( - state: NotificationsState, - action: NotificationsActions -): NotificationsState => { - const topicState = state[action.topic] as TopicNotificationsState | undefined - function getTopicState(notifications: NotifyClientTypes.NotifyNotification[]) { - const ids = topicState?.existingIds || new Set() - const filteredNotifications = notifications.filter(val => !ids.has(val.id)) - const notificationIds = notifications.map(notification => notification.id) +function getUpdatedTopicState( + topicState: TopicNotificationsState | undefined, + notifications: NotifyClientTypes.NotifyNotification[] +) { + const existingIds = topicState?.existingIds || new Set() + const filteredNotifications = notifications.filter(notification => !existingIds.has(notification.id)) - const fullNotifications = topicState?.fullNotifications || [] - const newFullIdsSet = new Set(topicState?.existingIds || []) + const fullNotifications = topicState?.fullNotifications || [] + const updatedIds = new Set(existingIds) - for (const val of notificationIds) { - newFullIdsSet.add(val) - } + notifications.forEach(notification => updatedIds.add(notification.id)) - return { - filteredNotifications, - fullNotifications, - newFullIdsSet - } + return { + filteredNotifications, + fullNotifications, + updatedIds } +} + +export const notificationsReducer = ( + state: NotificationsState, + action: NotificationsActions +): NotificationsState => { + const topicState = state[action.topic] switch (action.type) { case 'FETCH_NOTIFICATIONS_LOADING': { - if (topicState) { - return { - ...state, - [action.topic]: { - ...topicState, - isLoading: true + return topicState + ? { + ...state, + [action.topic]: { + ...topicState, + isLoading: true + } } - } - } - return state + : state } + case 'UNSHIFT_NEW_NOTIFICATIONS': { - const { filteredNotifications, fullNotifications, newFullIdsSet } = getTopicState( + const { filteredNotifications, fullNotifications, updatedIds } = getUpdatedTopicState( + topicState, action.notifications ) - const unshiftedNotifications = filteredNotifications.concat(fullNotifications) + const unshiftedNotifications = [...filteredNotifications, ...fullNotifications] return { ...state, [action.topic]: { ...topicState, - existingIds: newFullIdsSet, + existingIds: updatedIds, fullNotifications: unshiftedNotifications, hasMore: topicState?.hasMore || false, isLoading: false } } } + case 'FETCH_NOTIFICATIONS_DONE': { - const { filteredNotifications, fullNotifications, newFullIdsSet } = getTopicState( + const { filteredNotifications, fullNotifications, updatedIds } = getUpdatedTopicState( + topicState, action.notifications ) - const concatenatedNotification = fullNotifications.concat(filteredNotifications) + const concatenatedNotifications = [...fullNotifications, ...filteredNotifications] return { ...state, [action.topic]: { ...topicState, - existingIds: newFullIdsSet, - fullNotifications: concatenatedNotification, + existingIds: updatedIds, + fullNotifications: concatenatedNotifications, hasMore: action.hasMore, isLoading: false } } } + + default: + return state } } From 543e0bee244b466bea5f7c21a1b7f5e52a6c3034 Mon Sep 17 00:00:00 2001 From: Cypher Pepe <125112044+cypherpepe@users.noreply.github.com> Date: Fri, 18 Oct 2024 19:23:40 +0300 Subject: [PATCH 2/2] Added comments Added comments to notificationsReducer for clarity --- src/reducers/notifications.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/reducers/notifications.ts b/src/reducers/notifications.ts index 234c1e42..234ed441 100644 --- a/src/reducers/notifications.ts +++ b/src/reducers/notifications.ts @@ -1,5 +1,6 @@ import { NotifyClientTypes } from '@walletconnect/notify-client' +// Notification state for each topic export interface TopicNotificationsState { fullNotifications: NotifyClientTypes.NotifyNotification[] existingIds: Set @@ -28,13 +29,13 @@ export type NotificationsActions = topic: string } - +// Updates the topic state, filters notifications function getUpdatedTopicState( topicState: TopicNotificationsState | undefined, notifications: NotifyClientTypes.NotifyNotification[] ) { - const existingIds = topicState?.existingIds || new Set() - const filteredNotifications = notifications.filter(notification => !existingIds.has(notification.id)) + const existingIds = topicState?.existingIds || new Set() // Set of IDs for already existing notifications + const filteredNotifications = notifications.filter(notification => !existingIds.has(notification.id)) // Filters out new notifications const fullNotifications = topicState?.fullNotifications || [] const updatedIds = new Set(existingIds) @@ -48,6 +49,7 @@ function getUpdatedTopicState( } } +// Reducer for handling notification actions export const notificationsReducer = ( state: NotificationsState, action: NotificationsActions @@ -56,6 +58,7 @@ export const notificationsReducer = ( switch (action.type) { case 'FETCH_NOTIFICATIONS_LOADING': { + // Sets loading flag for the topic return topicState ? { ...state, @@ -68,6 +71,7 @@ export const notificationsReducer = ( } case 'UNSHIFT_NEW_NOTIFICATIONS': { + // Adds new notifications to the beginning of the list const { filteredNotifications, fullNotifications, updatedIds } = getUpdatedTopicState( topicState, action.notifications @@ -87,6 +91,7 @@ export const notificationsReducer = ( } case 'FETCH_NOTIFICATIONS_DONE': { + // Completes the loading of notifications const { filteredNotifications, fullNotifications, updatedIds } = getUpdatedTopicState( topicState, action.notifications