Improve gap handling

app/javascript/mastodon/actions/notification_groups.ts

@@ -73,17 +73,15 @@ export const fetchNotifications = createDataLoadingThunk(
           : excludeAllTypesExcept(activeFilter),
     });
   },
-  ({ notifications, links }, { dispatch }) => {
+  ({ notifications }, { dispatch }) => {
     dispatchAssociatedRecords(dispatch, notifications);
-
-    // We ignore the previous link, as it will always be here but we know there are no more
-    // recent notifications when doing the initial load
-    const nextLink = links.refs.find((link) => link.rel === 'next');
-
     const payload: (ApiNotificationGroupJSON | NotificationGap)[] =
       notifications;
 
-    if (nextLink) payload.push({ type: 'gap', loadUrl: nextLink.uri });
+    // TODO: might be worth not using gaps for that…
+    // if (nextLink) payload.push({ type: 'gap', loadUrl: nextLink.uri });
+    if (notifications.length > 1)
+      payload.push({ type: 'gap', maxId: notifications.at(-1)?.page_min_id });
 
     return payload;
     // dispatch(submitMarkers());
@@ -93,14 +91,12 @@ export const fetchNotifications = createDataLoadingThunk(
 export const fetchNotificationsGap = createDataLoadingThunk(
   'notificationGroups/fetchGat',
   async (params: { gap: NotificationGap }) =>
-    apiFetchNotifications({}, params.gap.loadUrl),
+    apiFetchNotifications({ max_id: params.gap.maxId }),
 
-  ({ notifications, links }, { dispatch }) => {
+  ({ notifications }, { dispatch }) => {
     dispatchAssociatedRecords(dispatch, notifications);
 
-    const nextLink = links.refs.find((link) => link.rel === 'next');
-
-    return { notifications, nextLink };
+    return { notifications };
   },
 );
 

app/javascript/mastodon/api/notifications.ts

@@ -1,15 +1,13 @@
 import api, { apiRequest, getLinks } from 'mastodon/api';
 import type { ApiNotificationGroupJSON } from 'mastodon/api_types/notifications';
 
-export const apiFetchNotifications = async (
-  params?: {
-    exclude_types?: string[];
-  },
-  forceUrl?: string,
-) => {
+export const apiFetchNotifications = async (params?: {
+  exclude_types?: string[];
+  max_id?: string;
+}) => {
   const response = await api().request<ApiNotificationGroupJSON[]>({
     method: 'GET',
-    url: forceUrl ?? '/api/v2_alpha/notifications',
+    url: '/api/v2_alpha/notifications',
     params,
   });
 

app/javascript/mastodon/features/notifications_v2/index.tsx

@@ -33,7 +33,6 @@ import type { RootState } from 'mastodon/store';
 import { addColumn, removeColumn, moveColumn } from '../../actions/columns';
 import { submitMarkers } from '../../actions/markers';
 import {
-  expandNotifications,
   scrollTopNotifications,
   loadPending,
   // mountNotifications,
@@ -95,7 +94,7 @@ export const Notifications: React.FC<{
   const notifications = useAppSelector(getNotifications);
   const dispatch = useAppDispatch();
   const isLoading = useAppSelector((s) => s.notificationsGroups.isLoading);
-  const hasMore = useAppSelector((s) => s.notificationsGroups.hasMore);
+  const hasMore = notifications.at(-1)?.type === 'gap';
 
   const lastReadId = useAppSelector((s) =>
     selectSettingsNotificationsShowUnread(s) ? s.markers.notifications : '0',
@@ -159,12 +158,10 @@ export const Notifications: React.FC<{
     [dispatch],
   );
 
-  // TODO: fix this, probably incorrect
   const handleLoadOlder = useDebouncedCallback(
     () => {
-      const last = notifications[notifications.length - 1];
-      if (last && last.type !== 'gap')
-        dispatch(expandNotifications({ maxId: last.group_key }));
+      const gap = notifications.at(-1);
+      if (gap?.type === 'gap') void dispatch(fetchNotificationsGap({ gap }));
     },
     300,
     { leading: true },
@@ -254,7 +251,7 @@ export const Notifications: React.FC<{
     return notifications.map((item) =>
       item.type === 'gap' ? (
         <LoadGap
-          key={item.loadUrl}
+          key={`${item.maxId}-${item.sinceId}`}
           disabled={isLoading}
           param={item}
           onClick={handleLoadGap}

app/javascript/mastodon/reducers/notifications_groups.ts

@@ -17,6 +17,7 @@ import {
   disconnectTimeline,
   timelineDelete,
 } from 'mastodon/actions/timelines_typed';
+import { compareId } from 'mastodon/compare_id';
 import {
   NOTIFICATIONS_GROUP_MAX_AVATARS,
   createNotificationGroupFromJSON,
@@ -26,19 +27,18 @@ import type { NotificationGroup } from 'mastodon/models/notification_group';
 
 export interface NotificationGap {
   type: 'gap';
-  loadUrl: string;
+  maxId?: string;
+  sinceId?: string;
 }
 
 interface NotificationGroupsState {
   groups: (NotificationGroup | NotificationGap)[];
   isLoading: boolean;
-  hasMore: boolean;
 }
 
 const initialState: NotificationGroupsState = {
   groups: [],
   isLoading: false,
-  hasMore: false,
 };
 
 function removeNotificationsForAccounts(
@@ -69,6 +69,7 @@ function removeNotificationsForAccounts(
     .filter(
       (group) => group.type === 'gap' || group.sampleAccountsIds.length > 0,
     );
+  mergeGaps(state.groups);
 }
 
 function removeNotificationsForStatus(
@@ -81,6 +82,61 @@ function removeNotificationsForStatus(
       !('statusId' in group) ||
       group.statusId !== statusId,
   );
+  mergeGaps(state.groups);
+}
+
+function isNotificationGroup(
+  groupOrGap: NotificationGroup | NotificationGap,
+): groupOrGap is NotificationGroup {
+  return groupOrGap.type !== 'gap';
+}
+
+// Merge adjacent gaps in `groups` in-place
+function mergeGaps(groups: NotificationGroupsState['groups']) {
+  for (let i = 0; i < groups.length; i++) {
+    const firstGroupOrGap = groups[i];
+
+    if (firstGroupOrGap?.type === 'gap') {
+      let lastGap = firstGroupOrGap;
+      let j = i + 1;
+
+      for (; j < groups.length; j++) {
+        const groupOrGap = groups[j];
+        if (groupOrGap?.type === 'gap') lastGap = groupOrGap;
+        else break;
+      }
+
+      if (j - i > 1) {
+        groups.splice(i, j - i, {
+          type: 'gap',
+          maxId: firstGroupOrGap.maxId,
+          sinceId: lastGap.sinceId,
+        });
+      }
+    }
+  }
+}
+
+// Checks if `groups[index-1]` and `groups[index]` are gaps, and merge them in-place if they are
+function mergeGapsAround(
+  groups: NotificationGroupsState['groups'],
+  index: number,
+) {
+  if (index > 0) {
+    const potentialFirstGap = groups[index - 1];
+    const potentialSecondGap = groups[index];
+
+    if (
+      potentialFirstGap?.type === 'gap' &&
+      potentialSecondGap?.type === 'gap'
+    ) {
+      groups.splice(index - 1, 2, {
+        type: 'gap',
+        maxId: potentialFirstGap.maxId,
+        sinceId: potentialSecondGap.sinceId,
+      });
+    }
+  }
 }
 
 export const notificationsGroupsReducer =
@@ -93,34 +149,83 @@ export const notificationsGroupsReducer =
         state.isLoading = false;
       })
       .addCase(fetchNotificationsGap.fulfilled, (state, action) => {
-        const { notifications, nextLink } = action.payload;
+        const { notifications } = action.payload;
 
         // find the gap in the existing notifications
         const gapIndex = state.groups.findIndex(
           (groupOrGap) =>
-            groupOrGap.type === 'gap' && groupOrGap.loadUrl === nextLink?.uri,
+            groupOrGap.type === 'gap' &&
+            groupOrGap.sinceId === action.meta.arg.gap.sinceId &&
+            groupOrGap.maxId === action.meta.arg.gap.maxId,
         );
 
-        if (!gapIndex)
+        if (gapIndex < 0)
           // We do not know where to insert, let's return
           return;
 
+        // Filling a disconnection gap means we're getting historical data
+        // about groups we may know or may not know about.
+
+        // The notifications timeline is split in two by the gap, with
+        // group information newer than the gap, and group information older
+        // than the gap.
+
+        // Filling a gap should not touch anything before the gap, so any
+        // information on groups already appearing before the gap should be
+        // discarded, while any information on groups appearing after the gap
+        // can be updated and re-ordered.
+
+        const oldestPageNotification = notifications.at(-1)?.page_min_id;
+
         // replace the gap with the notifications + a new gap
 
-        const toInsert: NotificationGroupsState['groups'] = notifications.map(
-          (json) => createNotificationGroupFromJSON(json),
+        const newerGroupKeys = state.groups
+          .slice(0, gapIndex)
+          .filter(isNotificationGroup)
+          .map((group) => group.group_key);
+
+        const toInsert: NotificationGroupsState['groups'] = notifications
+          .map((json) => createNotificationGroupFromJSON(json))
+          .filter(
+            (notification) => !newerGroupKeys.includes(notification.group_key),
+          );
+
+        const apiGroupKeys = (toInsert as NotificationGroup[]).map(
+          (group) => group.group_key,
         );
 
-        if (nextLink?.uri && notifications.length > 0) {
+        const sinceId = action.meta.arg.gap.sinceId;
+        if (
+          notifications.length > 0 &&
+          !(
+            oldestPageNotification &&
+            sinceId &&
+            compareId(oldestPageNotification, sinceId) <= 0
+          )
+        ) {
           // If we get an empty page, it means we reached the bottom, so we do not need to insert a new gap
+          // Similarly, if we've fetched more than the gap's, this means we have completely filled it
           toInsert.push({
             type: 'gap',
-            loadUrl: nextLink.uri,
+            maxId: notifications.at(-1)?.page_max_id,
+            sinceId,
           } as NotificationGap);
         }
 
+        // Remove older groups covered by the API
+        state.groups = state.groups.filter(
+          (groupOrGap) =>
+            groupOrGap.type !== 'gap' &&
+            !apiGroupKeys.includes(groupOrGap.group_key),
+        );
+
+        // Replace the gap with API results (+ the new gap if needed)
         state.groups.splice(gapIndex, 1, ...toInsert);
 
+        // Finally, merge any adjacent gaps that could have been created by filtering
+        // groups earlier
+        mergeGaps(state.groups);
+
         state.isLoading = false;
       })
       .addCase(processNewNotificationForGroups.fulfilled, (state, action) => {
@@ -130,6 +235,12 @@ export const notificationsGroupsReducer =
             group.type !== 'gap' && group.group_key === notification.group_key,
         );
 
+        // In any case, we are going to add a group at the top
+        // If there is currently a gap at the top, now is the time to update it
+        if (state.groups.length > 0 && state.groups[0]?.type === 'gap') {
+          state.groups[0].maxId = notification.id;
+        }
+
         if (existingGroupIndex > -1) {
           const existingGroup = state.groups[existingGroupIndex];
 
@@ -151,6 +262,8 @@ export const notificationsGroupsReducer =
             existingGroup.notifications_count += 1;
 
             state.groups.splice(existingGroupIndex, 1);
+            mergeGapsAround(state.groups, existingGroupIndex);
+
             state.groups.unshift(existingGroup);
           }
         } else {
@@ -162,13 +275,12 @@ export const notificationsGroupsReducer =
       })
       .addCase(disconnectTimeline, (state, action) => {
         if (action.payload.timeline === 'home') {
-          if (state.groups.length > 0 && state.groups[0]?.type === 'gap')
-            state.groups.shift();
-
-          state.groups.unshift({
-            type: 'gap',
-            loadUrl: 'TODO_LOAD_URL_TOP_OF_TL', // TODO
-          });
+          if (state.groups.length > 0 && state.groups[0]?.type !== 'gap') {
+            state.groups.unshift({
+              type: 'gap',
+              sinceId: state.groups[0]?.page_min_id,
+            });
+          }
         }
       })
       .addCase(timelineDelete, (state, action) => {
@@ -176,7 +288,6 @@ export const notificationsGroupsReducer =
       })
       .addCase(clearNotifications.pending, (state) => {
         state.groups = [];
-        state.hasMore = false;
       })
       .addCase(blockAccountSuccess, (state, action) => {
         removeNotificationsForAccounts(state, [action.payload.relationship.id]);