From 64a68c650e15bb1504ea7772f1defe10a91410b2 Mon Sep 17 00:00:00 2001 From: Alex Gleason Date: Tue, 22 Sep 2020 13:42:08 -0500 Subject: [PATCH] Notifications: refactor with OrderedSet instead of List --- app/soapbox/actions/notifications.js | 17 +++-- app/soapbox/features/notifications/index.js | 4 +- .../reducers/__tests__/notifications-test.js | 68 ++++++++--------- app/soapbox/reducers/notifications.js | 76 +++++++++---------- 4 files changed, 83 insertions(+), 82 deletions(-) diff --git a/app/soapbox/actions/notifications.js b/app/soapbox/actions/notifications.js index 1346c36c0..96bdd09dc 100644 --- a/app/soapbox/actions/notifications.js +++ b/app/soapbox/actions/notifications.js @@ -10,7 +10,11 @@ import { } from './importer'; import { getSettings, saveSettings } from './settings'; import { defineMessages } from 'react-intl'; -import { List as ImmutableList } from 'immutable'; +import { + List as ImmutableList, + Map as ImmutableMap, + OrderedSet as ImmutableOrderedSet, +} from 'immutable'; import { unescapeHTML } from '../utils/html'; import { getFilters, regexFromFilters } from '../selectors'; @@ -121,7 +125,7 @@ export function updateNotificationsQueue(notification, intlMessages, intlLocale, export function dequeueNotifications() { return (dispatch, getState) => { - const queuedNotifications = getState().getIn(['notifications', 'queuedNotifications'], ImmutableList()); + const queuedNotifications = getState().getIn(['notifications', 'queuedNotifications'], ImmutableOrderedSet()); const totalQueuedNotificationsCount = getState().getIn(['notifications', 'totalQueuedNotificationsCount'], 0); if (totalQueuedNotificationsCount === 0) { @@ -252,9 +256,12 @@ export function setFilter(filterType) { export function markReadNotifications() { return (dispatch, getState) => { - if (!getState().get('me')) return; - const topNotification = parseInt(getState().getIn(['notifications', 'items', 0, 'id'])); - const lastRead = getState().getIn(['notifications', 'lastRead']); + const state = getState(); + if (!state.get('me')) return; + + const topNotification = state.getIn(['notifications', 'items'], ImmutableOrderedSet()).first(ImmutableMap()).get('id'); + const lastRead = state.getIn(['notifications', 'lastRead']); + if (!(topNotification && topNotification > lastRead)) return; dispatch({ diff --git a/app/soapbox/features/notifications/index.js b/app/soapbox/features/notifications/index.js index 88a980369..89ccea75a 100644 --- a/app/soapbox/features/notifications/index.js +++ b/app/soapbox/features/notifications/index.js @@ -55,7 +55,7 @@ export default @connect(mapStateToProps) class Notifications extends React.PureComponent { static propTypes = { - notifications: ImmutablePropTypes.list.isRequired, + notifications: ImmutablePropTypes.orderedSet.isRequired, showFilterBar: PropTypes.bool.isRequired, dispatch: PropTypes.func.isRequired, intl: PropTypes.object.isRequired, @@ -142,6 +142,8 @@ class Notifications extends React.PureComponent { } else if (notifications.size > 0 || hasMore) { scrollableContent = notifications.map((item, index) => item === null ? ( 0 ? notifications.getIn([index - 1, 'id']) : null} diff --git a/app/soapbox/reducers/__tests__/notifications-test.js b/app/soapbox/reducers/__tests__/notifications-test.js index 97141c756..63f72d359 100644 --- a/app/soapbox/reducers/__tests__/notifications-test.js +++ b/app/soapbox/reducers/__tests__/notifications-test.js @@ -1,7 +1,7 @@ import * as actions from 'soapbox/actions/notifications'; import reducer from '../notifications'; import notifications from 'soapbox/__fixtures__/notifications.json'; -import { Map as ImmutableMap, List as ImmutableList } from 'immutable'; +import { Map as ImmutableMap, OrderedSet as ImmutableOrderedSet } from 'immutable'; import { take } from 'lodash'; import { ACCOUNT_BLOCK_SUCCESS, ACCOUNT_MUTE_SUCCESS } from 'soapbox/actions/accounts'; import notification from 'soapbox/__fixtures__/notification.json'; @@ -12,12 +12,12 @@ import { TIMELINE_DELETE, TIMELINE_DISCONNECT } from 'soapbox/actions/timelines' describe('notifications reducer', () => { it('should return the initial state', () => { expect(reducer(undefined, {})).toEqual(ImmutableMap({ - items: ImmutableList(), + items: ImmutableOrderedSet(), hasMore: true, top: false, unread: 0, isLoading: false, - queuedNotifications: ImmutableList(), + queuedNotifications: ImmutableOrderedSet(), totalQueuedNotificationsCount: 0, lastRead: -1, })); @@ -32,7 +32,7 @@ describe('notifications reducer', () => { skipLoading: true, }; expect(reducer(state, action)).toEqual(ImmutableMap({ - items: ImmutableList([ + items: ImmutableOrderedSet([ ImmutableMap({ id: '10744', type: 'pleroma:emoji_reaction', @@ -68,7 +68,7 @@ describe('notifications reducer', () => { top: false, unread: 1, isLoading: false, - queuedNotifications: ImmutableList(), + queuedNotifications: ImmutableOrderedSet(), totalQueuedNotificationsCount: 0, lastRead: -1, })); @@ -100,7 +100,7 @@ describe('notifications reducer', () => { it('should handle NOTIFICATIONS_FILTER_SET', () => { const state = ImmutableMap({ - items: ImmutableList([ + items: ImmutableOrderedSet([ ImmutableMap({ id: '10744', type: 'pleroma:emoji_reaction', @@ -136,7 +136,7 @@ describe('notifications reducer', () => { top: false, unread: 1, isLoading: false, - queuedNotifications: ImmutableList(), + queuedNotifications: ImmutableOrderedSet(), totalQueuedNotificationsCount: 0, lastRead: -1, }); @@ -144,12 +144,12 @@ describe('notifications reducer', () => { type: actions.NOTIFICATIONS_FILTER_SET, }; expect(reducer(state, action)).toEqual(ImmutableMap({ - items: ImmutableList(), + items: ImmutableOrderedSet(), hasMore: true, top: false, unread: 1, isLoading: false, - queuedNotifications: ImmutableList(), + queuedNotifications: ImmutableOrderedSet(), totalQueuedNotificationsCount: 0, lastRead: -1, })); @@ -185,7 +185,7 @@ describe('notifications reducer', () => { it('should handle NOTIFICATIONS_UPDATE, when top = false, increment unread', () => { const state = ImmutableMap({ - items: ImmutableList(), + items: ImmutableOrderedSet(), top: false, unread: 1, }); @@ -194,7 +194,7 @@ describe('notifications reducer', () => { notification: notification, }; expect(reducer(state, action)).toEqual(ImmutableMap({ - items: ImmutableList([ + items: ImmutableOrderedSet([ ImmutableMap({ id: '10743', type: 'favourite', @@ -213,8 +213,8 @@ describe('notifications reducer', () => { it('should handle NOTIFICATIONS_UPDATE_QUEUE', () => { const state = ImmutableMap({ - items: ImmutableList([]), - queuedNotifications: ImmutableList([]), + items: ImmutableOrderedSet([]), + queuedNotifications: ImmutableOrderedSet([]), totalQueuedNotificationsCount: 0, }); const action = { @@ -224,8 +224,8 @@ describe('notifications reducer', () => { intlLocale: 'en', }; expect(reducer(state, action)).toEqual(ImmutableMap({ - items: ImmutableList([]), - queuedNotifications: ImmutableList([{ + items: ImmutableOrderedSet([]), + queuedNotifications: ImmutableOrderedSet([{ notification: notification, intlMessages: intlMessages, intlLocale: 'en', @@ -236,7 +236,7 @@ describe('notifications reducer', () => { it('should handle NOTIFICATIONS_DEQUEUE', () => { const state = ImmutableMap({ - items: ImmutableList([]), + items: ImmutableOrderedSet([]), queuedNotifications: take(notifications, 1), totalQueuedNotificationsCount: 1, }); @@ -244,15 +244,15 @@ describe('notifications reducer', () => { type: actions.NOTIFICATIONS_DEQUEUE, }; expect(reducer(state, action)).toEqual(ImmutableMap({ - items: ImmutableList([]), - queuedNotifications: ImmutableList([]), + items: ImmutableOrderedSet([]), + queuedNotifications: ImmutableOrderedSet([]), totalQueuedNotificationsCount: 0, })); }); it('should handle NOTIFICATIONS_EXPAND_SUCCESS with non-empty items and next set true', () => { const state = ImmutableMap({ - items: ImmutableList([ + items: ImmutableOrderedSet([ ImmutableMap({ id: '10734', type: 'pleroma:emoji_reaction', @@ -274,7 +274,7 @@ describe('notifications reducer', () => { next: true, }; expect(reducer(state, action)).toEqual(ImmutableMap({ - items: ImmutableList([ + items: ImmutableOrderedSet([ ImmutableMap({ id: '10744', type: 'pleroma:emoji_reaction', @@ -324,7 +324,7 @@ describe('notifications reducer', () => { it('should handle NOTIFICATIONS_EXPAND_SUCCESS with empty items and next set true', () => { const state = ImmutableMap({ - items: ImmutableList([]), + items: ImmutableOrderedSet([]), unread: 1, hasMore: true, isLoading: false, @@ -335,7 +335,7 @@ describe('notifications reducer', () => { next: true, }; expect(reducer(state, action)).toEqual(ImmutableMap({ - items: ImmutableList([ + items: ImmutableOrderedSet([ ImmutableMap({ id: '10744', type: 'pleroma:emoji_reaction', @@ -375,7 +375,7 @@ describe('notifications reducer', () => { it('should handle ACCOUNT_BLOCK_SUCCESS', () => { const state = ImmutableMap({ - items: ImmutableList([ + items: ImmutableOrderedSet([ ImmutableMap({ id: '10744', type: 'pleroma:emoji_reaction', @@ -413,7 +413,7 @@ describe('notifications reducer', () => { relationship: relationship, }; expect(reducer(state, action)).toEqual(ImmutableMap({ - items: ImmutableList([ + items: ImmutableOrderedSet([ ImmutableMap({ id: '10743', type: 'favourite', @@ -440,7 +440,7 @@ describe('notifications reducer', () => { it('should handle ACCOUNT_MUTE_SUCCESS', () => { const state = ImmutableMap({ - items: ImmutableList([ + items: ImmutableOrderedSet([ ImmutableMap({ id: '10744', type: 'pleroma:emoji_reaction', @@ -478,7 +478,7 @@ describe('notifications reducer', () => { relationship: relationship, }; expect(reducer(state, action)).toEqual(ImmutableMap({ - items: ImmutableList([ + items: ImmutableOrderedSet([ ImmutableMap({ id: '10743', type: 'favourite', @@ -505,35 +505,35 @@ describe('notifications reducer', () => { it('should handle NOTIFICATIONS_CLEAR', () => { const state = ImmutableMap({ - items: ImmutableList([]), + items: ImmutableOrderedSet([]), hasMore: true, }); const action = { type: actions.NOTIFICATIONS_CLEAR, }; expect(reducer(state, action)).toEqual(ImmutableMap({ - items: ImmutableList([]), + items: ImmutableOrderedSet([]), hasMore: false, })); }); it('should handle NOTIFICATIONS_MARK_READ_REQUEST', () => { const state = ImmutableMap({ - items: ImmutableList([]), + items: ImmutableOrderedSet([]), }); const action = { type: actions.NOTIFICATIONS_MARK_READ_REQUEST, lastRead: 35098814, }; expect(reducer(state, action)).toEqual(ImmutableMap({ - items: ImmutableList([]), + items: ImmutableOrderedSet([]), lastRead: 35098814, })); }); it('should handle TIMELINE_DELETE', () => { const state = ImmutableMap({ - items: ImmutableList([ + items: ImmutableOrderedSet([ ImmutableMap({ id: '10744', type: 'pleroma:emoji_reaction', @@ -571,13 +571,13 @@ describe('notifications reducer', () => { id: '9vvNxoo5EFbbnfdXQu', }; expect(reducer(state, action)).toEqual(ImmutableMap({ - items: ImmutableList([]), + items: ImmutableOrderedSet([]), })); }); it('should handle TIMELINE_DISCONNECT', () => { const state = ImmutableMap({ - items: ImmutableList([ + items: ImmutableOrderedSet([ ImmutableMap({ id: '10744', type: 'pleroma:emoji_reaction', @@ -615,7 +615,7 @@ describe('notifications reducer', () => { timeline: 'home', }; expect(reducer(state, action)).toEqual(ImmutableMap({ - items: ImmutableList([ + items: ImmutableOrderedSet([ null, ImmutableMap({ id: '10744', diff --git a/app/soapbox/reducers/notifications.js b/app/soapbox/reducers/notifications.js index e121c4e82..884e7f6f6 100644 --- a/app/soapbox/reducers/notifications.js +++ b/app/soapbox/reducers/notifications.js @@ -16,21 +16,27 @@ import { ACCOUNT_MUTE_SUCCESS, } from '../actions/accounts'; import { TIMELINE_DELETE, TIMELINE_DISCONNECT } from '../actions/timelines'; -import { Map as ImmutableMap, List as ImmutableList } from 'immutable'; -import compareId from '../compare_id'; +import { Map as ImmutableMap, OrderedSet as ImmutableOrderedSet } from 'immutable'; import { get } from 'lodash'; const initialState = ImmutableMap({ - items: ImmutableList(), + items: ImmutableOrderedSet(), hasMore: true, top: false, unread: 0, isLoading: false, - queuedNotifications: ImmutableList(), //max = MAX_QUEUED_NOTIFICATIONS + queuedNotifications: ImmutableOrderedSet(), //max = MAX_QUEUED_NOTIFICATIONS totalQueuedNotificationsCount: 0, //used for queuedItems overflow for MAX_QUEUED_NOTIFICATIONS+ lastRead: -1, }); +// For sorting the notifications +const comparator = (a, b) => { + if (a.get('id') < b.get('id')) return 1; + if (a.get('id') > b.get('id')) return -1; + return 0; +}; + const notificationToMap = notification => ImmutableMap({ id: notification.id, type: notification.type, @@ -42,49 +48,36 @@ const notificationToMap = notification => ImmutableMap({ is_seen: get(notification, ['pleroma', 'is_seen'], true), }); +// https://gitlab.com/soapbox-pub/soapbox-fe/-/issues/424 +const isValid = notification => Boolean(notification.account.id); + const normalizeNotification = (state, notification) => { const top = state.get('top'); - if (!top) { - state = state.update('unread', unread => unread + 1); - } + if (!top) state = state.update('unread', unread => unread + 1); return state.update('items', list => { if (top && list.size > 40) { list = list.take(20); } - return list.unshift(notificationToMap(notification)); + return list.add(notificationToMap(notification)).sort(comparator); }); }; -const expandNormalizedNotifications = (state, notifications, next) => { - let items = ImmutableList(); +const processRawNotifications = notifications => ( + ImmutableOrderedSet( + notifications + .filter(isValid) + .map(notificationToMap))); - notifications.forEach((n) => { - if (!n.account.id) return; - items = items.push(notificationToMap(n)); - }); +const expandNormalizedNotifications = (state, notifications, next) => { + const items = processRawNotifications(notifications); return state.withMutations(mutable => { - if (!items.isEmpty()) { - mutable.update('items', list => { - const lastIndex = 1 + list.findLastIndex( - item => item !== null && (compareId(item.get('id'), items.last().get('id')) > 0 || item.get('id') === items.last().get('id')) - ); - - const firstIndex = 1 + list.take(lastIndex).findLastIndex( - item => item !== null && compareId(item.get('id'), items.first().get('id')) > 0 - ); - - return list.take(firstIndex).concat(items, list.skip(lastIndex)); - }); - } - - if (!next) { - mutable.set('hasMore', false); - } + mutable.update('items', list => list.union(items).sort(comparator)); + if (!next) mutable.set('hasMore', false); mutable.set('isLoading', false); }); }; @@ -94,10 +87,7 @@ const filterNotifications = (state, relationship) => { }; const updateTop = (state, top) => { - if (top) { - state = state.set('unread', 0); - } - + if (top) state = state.set('unread', 0); return state.set('top', top); }; @@ -106,8 +96,8 @@ const deleteByStatus = (state, statusId) => { }; const updateNotificationsQueue = (state, notification, intlMessages, intlLocale) => { - const queuedNotifications = state.getIn(['queuedNotifications'], ImmutableList()); - const listedNotifications = state.getIn(['items'], ImmutableList()); + const queuedNotifications = state.getIn(['queuedNotifications'], ImmutableOrderedSet()); + const listedNotifications = state.getIn(['items'], ImmutableOrderedSet()); const totalQueuedNotificationsCount = state.getIn(['totalQueuedNotificationsCount'], 0); let alreadyExists = queuedNotifications.find(existingQueuedNotification => existingQueuedNotification.id === notification.id); @@ -121,7 +111,7 @@ const updateNotificationsQueue = (state, notification, intlMessages, intlLocale) return state.withMutations(mutable => { if (totalQueuedNotificationsCount <= MAX_QUEUED_NOTIFICATIONS) { - mutable.set('queuedNotifications', newQueuedNotifications.push({ + mutable.set('queuedNotifications', newQueuedNotifications.add({ notification, intlMessages, intlLocale, @@ -138,7 +128,7 @@ export default function notifications(state = initialState, action) { case NOTIFICATIONS_EXPAND_FAIL: return state.set('isLoading', false); case NOTIFICATIONS_FILTER_SET: - return state.set('items', ImmutableList()).set('hasMore', true); + return state.set('items', ImmutableOrderedSet()).set('hasMore', true); case NOTIFICATIONS_SCROLL_TOP: return updateTop(state, action.top); case NOTIFICATIONS_UPDATE: @@ -147,7 +137,7 @@ export default function notifications(state = initialState, action) { return updateNotificationsQueue(state, action.notification, action.intlMessages, action.intlLocale); case NOTIFICATIONS_DEQUEUE: return state.withMutations(mutable => { - mutable.set('queuedNotifications', ImmutableList()); + mutable.set('queuedNotifications', ImmutableOrderedSet()); mutable.set('totalQueuedNotificationsCount', 0); }); case NOTIFICATIONS_EXPAND_SUCCESS: @@ -160,14 +150,16 @@ export default function notifications(state = initialState, action) { case ACCOUNT_MUTE_SUCCESS: return action.relationship.muting_notifications ? filterNotifications(state, action.relationship) : state; case NOTIFICATIONS_CLEAR: - return state.set('items', ImmutableList()).set('hasMore', false); + return state.set('items', ImmutableOrderedSet()).set('hasMore', false); case NOTIFICATIONS_MARK_READ_REQUEST: return state.set('lastRead', action.lastRead); case TIMELINE_DELETE: return deleteByStatus(state, action.id); case TIMELINE_DISCONNECT: + // This is kind of a hack - `null` renders a LoadGap in the component + // https://github.com/tootsuite/mastodon/pull/6886 return action.timeline === 'home' ? - state.update('items', items => items.first() ? items.unshift(null) : items) : + state.update('items', items => items.first() ? ImmutableOrderedSet([null]).union(items) : items) : state; default: return state;