From 3b064b269f97d25f899bb70de3f715084286095a Mon Sep 17 00:00:00 2001 From: Claire Date: Thu, 20 Jun 2024 12:36:45 +0200 Subject: [PATCH] Improve gap handling --- .../mastodon/actions/notification_groups.ts | 20 +-- app/javascript/mastodon/api/notifications.ts | 12 +- .../features/notifications_v2/index.tsx | 11 +- .../mastodon/reducers/notifications_groups.ts | 147 +++++++++++++++--- 4 files changed, 146 insertions(+), 44 deletions(-) diff --git a/app/javascript/mastodon/actions/notification_groups.ts b/app/javascript/mastodon/actions/notification_groups.ts index 464658851f..a9ddbf4397 100644 --- a/app/javascript/mastodon/actions/notification_groups.ts +++ b/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 }; }, ); diff --git a/app/javascript/mastodon/api/notifications.ts b/app/javascript/mastodon/api/notifications.ts index 315c477ba4..c1ab6f70ca 100644 --- a/app/javascript/mastodon/api/notifications.ts +++ b/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({ method: 'GET', - url: forceUrl ?? '/api/v2_alpha/notifications', + url: '/api/v2_alpha/notifications', params, }); diff --git a/app/javascript/mastodon/features/notifications_v2/index.tsx b/app/javascript/mastodon/features/notifications_v2/index.tsx index 471649d989..7ad9dbf1cc 100644 --- a/app/javascript/mastodon/features/notifications_v2/index.tsx +++ b/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' ? ( 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]);