From 04e0ebe8fc4ec32501cc4138e0357308a171807c Mon Sep 17 00:00:00 2001 From: Paul Frazee Date: Wed, 19 Apr 2023 20:11:10 -0500 Subject: [PATCH] Feed and notifs improvements (#498) * Reduce frequency of the notifications sync * Reduce frequency of home feed polling * Ensure loading spinner is visible in notifications * Render notifications loading spinner in the flatlist * Fixes and performance improvements to notifications * Render 30+ on notifications if at max * Fix issue with repeating posts in home feed * Dont check for unread notifs if we're already at max --- src/state/models/feeds/notifications.ts | 148 ++++++++++-------------- src/state/models/feeds/posts.ts | 47 +++----- src/state/models/me.ts | 7 +- src/view/com/notifications/Feed.tsx | 42 +++++-- src/view/screens/Home.tsx | 5 +- src/view/screens/Notifications.tsx | 5 +- src/view/shell/Drawer.tsx | 12 +- src/view/shell/bottom-bar/BottomBar.tsx | 6 +- src/view/shell/desktop/LeftNav.tsx | 8 +- 9 files changed, 130 insertions(+), 150 deletions(-) diff --git a/src/state/models/feeds/notifications.ts b/src/state/models/feeds/notifications.ts index 12db9510..ff77ab97 100644 --- a/src/state/models/feeds/notifications.ts +++ b/src/state/models/feeds/notifications.ts @@ -21,7 +21,7 @@ const MS_2DAY = MS_1HR * 48 let _idCounter = 0 -type CondFn = (notif: ListNotifications.Notification) => boolean +export const MAX_VISIBLE_NOTIFS = 30 export interface GroupedNotification extends ListNotifications.Notification { additional?: ListNotifications.Notification[] @@ -220,6 +220,7 @@ export class NotificationsFeedModel { loadMoreError = '' hasMore = true loadMoreCursor?: string + lastSync?: Date // used to linearize async modifications to state lock = new AwaitLock() @@ -259,6 +260,17 @@ export class NotificationsFeedModel { return this.queuedNotifications && this.queuedNotifications?.length > 0 } + get unreadCountLabel(): string { + const count = this.unreadCount + this.rootStore.invitedUsers.numNotifs + if (count >= MAX_VISIBLE_NOTIFS) { + return `${MAX_VISIBLE_NOTIFS}+` + } + if (count === 0) { + return '' + } + return String(count) + } + // public api // = @@ -288,10 +300,13 @@ export class NotificationsFeedModel { try { this._xLoading(isRefreshing) try { - const res = await this._fetchUntil(notif => notif.isRead, { - breakAt: 'page', + const res = await this.rootStore.agent.listNotifications({ + limit: PAGE_SIZE, }) await this._replaceAll(res) + runInAction(() => { + this.lastSync = new Date() + }) this._setQueued(undefined) this._countUnread() this._xIdle() @@ -313,20 +328,33 @@ export class NotificationsFeedModel { /** * Sync the next set of notifications to show - * returns true if the number changed */ syncQueue = bundleAsync(async () => { this.rootStore.log.debug('NotificationsModel:syncQueue') + if (this.unreadCount >= MAX_VISIBLE_NOTIFS) { + return // no need to check + } await this.lock.acquireAsync() try { - const res = await this._fetchUntil( - notif => - this.notifications.length - ? isEq(notif, this.notifications[0]) - : notif.isRead, - {breakAt: 'record'}, - ) - this._setQueued(res.data.notifications) + const res = await this.rootStore.agent.listNotifications({ + limit: PAGE_SIZE, + }) + + const queue = [] + for (const notif of res.data.notifications) { + if (this.notifications.length) { + if (isEq(notif, this.notifications[0])) { + break + } + } else { + if (!notif.isRead) { + break + } + } + queue.push(notif) + } + + this._setQueued(this._filterNotifications(queue)) this._countUnread() } catch (e) { this.rootStore.log.error('NotificationsModel:syncQueue failed', {e}) @@ -335,37 +363,6 @@ export class NotificationsFeedModel { } }) - /** - * - */ - processQueue = bundleAsync(async () => { - this.rootStore.log.debug('NotificationsModel:processQueue') - if (!this.queuedNotifications) { - return - } - this.isRefreshing = true - await this.lock.acquireAsync() - try { - runInAction(() => { - this.mostRecentNotificationUri = this.queuedNotifications?.[0].uri - }) - const itemModels = await this._processNotifications( - this.queuedNotifications, - ) - this._setQueued(undefined) - runInAction(() => { - this.notifications = itemModels.concat(this.notifications) - }) - } catch (e) { - this.rootStore.log.error('NotificationsModel:processQueue failed', {e}) - } finally { - runInAction(() => { - this.isRefreshing = false - }) - this.lock.release() - } - }) - /** * Load more posts to the end of the notifications */ @@ -423,22 +420,23 @@ export class NotificationsFeedModel { /** * Update read/unread state */ - async markAllUnqueuedRead() { + async markAllRead() { try { for (const notif of this.notifications) { notif.markGroupRead() } this._countUnread() - if (this.notifications[0]) { - await this.rootStore.agent.updateSeenNotifications( - this.notifications[0].indexedAt, - ) - } + await this.rootStore.agent.updateSeenNotifications( + this.lastSync ? this.lastSync.toISOString() : undefined, + ) } catch (e: any) { this.rootStore.log.warn('Failed to update notifications read state', e) } } + /** + * Used in background fetch to trigger notifications + */ async getNewMostRecent(): Promise { let old = this.mostRecentNotificationUri const res = await this.rootStore.agent.listNotifications({ @@ -486,40 +484,6 @@ export class NotificationsFeedModel { // helper functions // = - async _fetchUntil( - condFn: CondFn, - {breakAt}: {breakAt: 'page' | 'record'}, - ): Promise { - const accRes: ListNotifications.Response = { - success: true, - headers: {}, - data: {cursor: undefined, notifications: []}, - } - for (let i = 0; i <= 10; i++) { - const res = await this.rootStore.agent.listNotifications({ - limit: PAGE_SIZE, - cursor: accRes.data.cursor, - }) - accRes.data.cursor = res.data.cursor - - let pageIsDone = false - for (const notif of res.data.notifications) { - if (condFn(notif)) { - if (breakAt === 'record') { - return accRes - } else { - pageIsDone = true - } - } - accRes.data.notifications.push(notif) - } - if (pageIsDone || res.data.notifications.length < PAGE_SIZE) { - return accRes - } - } - return accRes - } - async _replaceAll(res: ListNotifications.Response) { if (res.data.notifications[0]) { this.mostRecentNotificationUri = res.data.notifications[0].uri @@ -540,17 +504,23 @@ export class NotificationsFeedModel { }) } - async _processNotifications( + _filterNotifications( items: ListNotifications.Notification[], - ): Promise { - const promises = [] - const itemModels: NotificationsFeedItemModel[] = [] - items = items.filter(item => { + ): ListNotifications.Notification[] { + return items.filter(item => { return ( this.rootStore.preferences.getLabelPreference(item.labels).pref !== 'hide' ) }) + } + + async _processNotifications( + items: ListNotifications.Notification[], + ): Promise { + const promises = [] + const itemModels: NotificationsFeedItemModel[] = [] + items = this._filterNotifications(items) for (const item of groupNotifications(items)) { const itemModel = new NotificationsFeedItemModel( this.rootStore, @@ -581,7 +551,7 @@ export class NotificationsFeedModel { unread += notif.numUnreadInGroup } if (this.queuedNotifications) { - unread += this.queuedNotifications.length + unread += this.queuedNotifications.filter(notif => !notif.isRead).length } this.unreadCount = unread this.rootStore.emitUnreadNotifications(unread) diff --git a/src/state/models/feeds/posts.ts b/src/state/models/feeds/posts.ts index e3328c71..38faf658 100644 --- a/src/state/models/feeds/posts.ts +++ b/src/state/models/feeds/posts.ts @@ -237,7 +237,6 @@ export class PostsFeedModel { // data slices: PostsFeedSliceModel[] = [] - nextSlices: PostsFeedSliceModel[] = [] constructor( public rootStore: RootStoreModel, @@ -309,7 +308,6 @@ export class PostsFeedModel { this.loadMoreCursor = undefined this.pollCursor = undefined this.slices = [] - this.nextSlices = [] this.tuner.reset() } @@ -461,46 +459,33 @@ export class PostsFeedModel { } const res = await this._getFeed({limit: PAGE_SIZE}) const tuner = new FeedTuner() - const nextSlices = tuner.tune(res.data.feed, this.feedTuners) - if (nextSlices[0]?.uri !== this.slices[0]?.uri) { - const nextSlicesModels = nextSlices.map( - slice => - new PostsFeedSliceModel( - this.rootStore, - `item-${_idCounter++}`, - slice, - ), - ) - if (autoPrepend) { + const slices = tuner.tune(res.data.feed, this.feedTuners) + if (slices[0]?.uri !== this.slices[0]?.uri) { + if (!autoPrepend) { + this.setHasNewLatest(true) + } else { + this.setHasNewLatest(false) runInAction(() => { - this.slices = nextSlicesModels.concat( + const slicesModels = slices.map( + slice => + new PostsFeedSliceModel( + this.rootStore, + `item-${_idCounter++}`, + slice, + ), + ) + this.slices = slicesModels.concat( this.slices.filter(slice1 => - nextSlicesModels.find(slice2 => slice1.uri === slice2.uri), + slicesModels.find(slice2 => slice1.uri === slice2.uri), ), ) - this.setHasNewLatest(false) }) - } else { - runInAction(() => { - this.nextSlices = nextSlicesModels - }) - this.setHasNewLatest(true) } } else { this.setHasNewLatest(false) } } - /** - * Sets the current slices to the "next slices" loaded by checkForLatest - */ - resetToLatest() { - if (this.nextSlices.length) { - this.slices = this.nextSlices - } - this.setHasNewLatest(false) - } - /** * Removes posts from the feed upon deletion. */ diff --git a/src/state/models/me.ts b/src/state/models/me.ts index 3774e1e5..e8b8e1ed 100644 --- a/src/state/models/me.ts +++ b/src/state/models/me.ts @@ -7,6 +7,7 @@ import {MyFollowsCache} from './cache/my-follows' import {isObj, hasProp} from 'lib/type-guards' const PROFILE_UPDATE_INTERVAL = 10 * 60 * 1e3 // 10min +const NOTIFS_UPDATE_INTERVAL = 30 * 1e3 // 30sec export class MeModel { did: string = '' @@ -21,6 +22,7 @@ export class MeModel { follows: MyFollowsCache invites: ComAtprotoServerDefs.InviteCode[] = [] lastProfileStateUpdate = Date.now() + lastNotifsUpdate = Date.now() get invitesAvailable() { return this.invites.filter(isInviteAvailable).length @@ -119,7 +121,10 @@ export class MeModel { await this.fetchProfile() await this.fetchInviteCodes() } - await this.notifications.syncQueue() + if (Date.now() - this.lastNotifsUpdate > NOTIFS_UPDATE_INTERVAL) { + this.lastNotifsUpdate = Date.now() + await this.notifications.syncQueue() + } } async fetchProfile() { diff --git a/src/view/com/notifications/Feed.tsx b/src/view/com/notifications/Feed.tsx index 23a3166d..33bde195 100644 --- a/src/view/com/notifications/Feed.tsx +++ b/src/view/com/notifications/Feed.tsx @@ -14,6 +14,7 @@ import {usePalette} from 'lib/hooks/usePalette' const EMPTY_FEED_ITEM = {_reactKey: '__empty__'} const LOAD_MORE_ERROR_ITEM = {_reactKey: '__load_more_error__'} +const LOADING_SPINNER = {_reactKey: '__loading_spinner__'} export const Feed = observer(function Feed({ view, @@ -27,28 +28,42 @@ export const Feed = observer(function Feed({ onScroll?: OnScrollCb }) { const pal = usePalette('default') + const [isPTRing, setIsPTRing] = React.useState(false) const data = React.useMemo(() => { - let feedItems + let feedItems: any[] = [] + if (view.isRefreshing && !isPTRing) { + feedItems = [LOADING_SPINNER] + } if (view.hasLoaded) { if (view.isEmpty) { - feedItems = [EMPTY_FEED_ITEM] + feedItems = feedItems.concat([EMPTY_FEED_ITEM]) } else { - feedItems = view.notifications + feedItems = feedItems.concat(view.notifications) } } if (view.loadMoreError) { feedItems = (feedItems || []).concat([LOAD_MORE_ERROR_ITEM]) } return feedItems - }, [view.hasLoaded, view.isEmpty, view.notifications, view.loadMoreError]) + }, [ + view.hasLoaded, + view.isEmpty, + view.notifications, + view.loadMoreError, + view.isRefreshing, + isPTRing, + ]) const onRefresh = React.useCallback(async () => { try { + setIsPTRing(true) await view.refresh() } catch (err) { view.rootStore.log.error('Failed to refresh notifications feed', err) + } finally { + setIsPTRing(false) } - }, [view]) + }, [view, setIsPTRing]) const onEndReached = React.useCallback(async () => { try { @@ -83,6 +98,12 @@ export const Feed = observer(function Feed({ onPress={onPressRetryLoadMore} /> ) + } else if (item === LOADING_SPINNER) { + return ( + + + + ) } return }, @@ -104,7 +125,9 @@ export const Feed = observer(function Feed({ return ( - {view.isLoading && !data && } + {view.isLoading && !data.length && ( + + )} {view.hasError && ( )} - {data && ( + {data.length && ( export const HomeScreen = withAuthRequired((_opts: Props) => { @@ -150,7 +151,7 @@ const FeedPage = observer( React.useCallback(() => { const softResetSub = store.onScreenSoftReset(onSoftReset) const feedCleanup = feed.registerListeners() - const pollInterval = setInterval(doPoll, 15e3) + const pollInterval = setInterval(doPoll, POLL_FREQ) screen('Feed') store.log.debug('HomeScreen: Updating feed') @@ -176,8 +177,8 @@ const FeedPage = observer( }, [feed]) const onPressLoadLatest = React.useCallback(() => { - feed.resetToLatest() scrollToTop() + feed.refresh() }, [feed, scrollToTop]) return ( diff --git a/src/view/screens/Notifications.tsx b/src/view/screens/Notifications.tsx index 76ad8161..3e34a9fa 100644 --- a/src/view/screens/Notifications.tsx +++ b/src/view/screens/Notifications.tsx @@ -38,8 +38,8 @@ export const NotificationsScreen = withAuthRequired( }, [scrollElRef]) const onPressLoadLatest = React.useCallback(() => { - store.me.notifications.processQueue() scrollToTop() + store.me.notifications.refresh() }, [store, scrollToTop]) // on-visible setup @@ -49,13 +49,12 @@ export const NotificationsScreen = withAuthRequired( store.shell.setMinimalShellMode(false) store.log.debug('NotificationsScreen: Updating feed') const softResetSub = store.onScreenSoftReset(onPressLoadLatest) - store.me.notifications.syncQueue() store.me.notifications.update() screen('Notifications') return () => { softResetSub.remove() - store.me.notifications.markAllUnqueuedRead() + store.me.notifications.markAllRead() } }, [store, screen, onPressLoadLatest]), ) diff --git a/src/view/shell/Drawer.tsx b/src/view/shell/Drawer.tsx index 74e10d6a..7128d421 100644 --- a/src/view/shell/Drawer.tsx +++ b/src/view/shell/Drawer.tsx @@ -203,9 +203,7 @@ export const DrawerContent = observer(() => { ) } label="Notifications" - count={ - store.me.notifications.unreadCount + store.invitedUsers.numNotifs - } + count={store.me.notifications.unreadCountLabel} bold={isAtNotifications} onPress={onPressNotifications} /> @@ -291,7 +289,7 @@ function MenuItem({ }: { icon: JSX.Element label: string - count?: number + count?: string bold?: boolean onPress: () => void }) { @@ -307,14 +305,14 @@ function MenuItem({ 99 + count.length > 2 ? styles.menuItemCountHundreds - : count > 9 + : count.length > 1 ? styles.menuItemCountTens : undefined, ]}> - {count > 999 ? `${Math.round(count / 1000)}k` : count} + {count} ) : undefined} diff --git a/src/view/shell/bottom-bar/BottomBar.tsx b/src/view/shell/bottom-bar/BottomBar.tsx index 4dcaf3eb..a7d11d81 100644 --- a/src/view/shell/bottom-bar/BottomBar.tsx +++ b/src/view/shell/bottom-bar/BottomBar.tsx @@ -132,9 +132,7 @@ export const BottomBar = observer(({navigation}: BottomTabBarProps) => { ) } onPress={onPressNotifications} - notificationCount={ - store.me.notifications.unreadCount + store.invitedUsers.numNotifs - } + notificationCount={store.me.notifications.unreadCountLabel} /> void onLongPress?: (event: GestureResponderEvent) => void }) { diff --git a/src/view/shell/desktop/LeftNav.tsx b/src/view/shell/desktop/LeftNav.tsx index c5486a8f..bcff844f 100644 --- a/src/view/shell/desktop/LeftNav.tsx +++ b/src/view/shell/desktop/LeftNav.tsx @@ -70,7 +70,7 @@ function BackBtn() { } interface NavItemProps { - count?: number + count?: string href: string icon: JSX.Element iconFilled: JSX.Element @@ -95,7 +95,7 @@ const NavItem = observer( {isCurrent ? iconFilled : icon} - {typeof count === 'number' && count > 0 && ( + {typeof count === 'string' && count && ( {count} @@ -162,9 +162,7 @@ export const DesktopLeftNav = observer(function DesktopLeftNav() { /> } iconFilled={