From 60681186430f4b7ab3b7268bf6c542714a21524b Mon Sep 17 00:00:00 2001 From: Claire Date: Thu, 20 Jun 2024 12:36:45 +0200 Subject: [PATCH] [WiP] Improve gap handling --- .../mastodon/actions/notification_groups.ts | 20 +++--- app/javascript/mastodon/api/notifications.ts | 12 ++-- .../features/notifications_v2/index.tsx | 2 +- .../mastodon/reducers/notifications_groups.ts | 68 ++++++++++++++----- 4 files changed, 66 insertions(+), 36 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..5061841f29 100644 --- a/app/javascript/mastodon/features/notifications_v2/index.tsx +++ b/app/javascript/mastodon/features/notifications_v2/index.tsx @@ -254,7 +254,7 @@ export const Notifications: React.FC<{ return notifications.map((item) => item.type === 'gap' ? ( { - 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 toInsert: NotificationGroupsState['groups'] = notifications + .map((json) => createNotificationGroupFromJSON(json)) + .filter( + // TODO: update notification groups instead of just filtering duplicates + (notification) => + state.groups.every( + (group) => + group.type === 'gap' || + group.group_key !== notification.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); } + // TODO: merge gaps made adjacent by moved/deleted groups state.groups.splice(gapIndex, 1, ...toInsert); state.isLoading = false; @@ -162,13 +199,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) => {