Notifications: refactor with OrderedSet instead of List

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({

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 ? (
         <LoadGap
+          // FIXME: This won't work, can't get OrderedSet by index
+          // https://github.com/immutable-js/immutable-js/issues/1790
           key={'gap:' + notifications.getIn([index + 1, 'id'])}
           disabled={isLoading}
           maxId={index > 0 ? notifications.getIn([index - 1, 'id']) : null}

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',

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;