From 133053fe9a2d8feafa4cdea456ae556f31d27b5f Mon Sep 17 00:00:00 2001 From: Pujit Mehrotra Date: Wed, 11 Dec 2024 12:08:45 -0500 Subject: [PATCH 1/2] fix(web): edge case where archived notifications don't appear if the archive has already been fetched/loaded. --- web/helpers/apollo-cache/index.ts | 63 +++++++++++++++++++++++++++---- 1 file changed, 55 insertions(+), 8 deletions(-) diff --git a/web/helpers/apollo-cache/index.ts b/web/helpers/apollo-cache/index.ts index c479091516..7e918b6737 100644 --- a/web/helpers/apollo-cache/index.ts +++ b/web/helpers/apollo-cache/index.ts @@ -1,6 +1,10 @@ import { InMemoryCache, type InMemoryCacheConfig } from '@apollo/client/core/index.js'; -import type { NotificationOverview } from '~/composables/gql/graphql'; -import { NotificationType } from '../../composables/gql/typename'; +import { + getNotifications, + NOTIFICATION_FRAGMENT, +} from '~/components/Notifications/graphql/notification.query'; +import { NotificationType, type NotificationOverview } from '~/composables/gql/graphql'; +import { NotificationType as NotificationCacheType } from '../../composables/gql/typename'; import { mergeAndDedup } from './merge'; /**------------------------------------------------------------------------ @@ -60,9 +64,9 @@ const defaultCacheConfig: InMemoryCacheConfig = { overview: { /** * Busts notification cache when new unread notifications are detected. - * + * * This allows incoming notifications to appear in the sidebar without reloading the page. - * + * * @param existing - Existing overview data in cache * @param incoming - New overview data from server * @param context - Apollo context containing cache instance @@ -106,6 +110,52 @@ const defaultCacheConfig: InMemoryCacheConfig = { return incoming; // Return the incoming data so Apollo knows the result of the mutation }, }, + archiveNotification: { + /** + * Ensures newly archived notifications appear in the archive list immediately. + * + * When a notification is archived, we need to manually prepend it to the archive list + * in the cache if that list has already been queried. Otherwise, the archived notification + * won't appear until the next refetch (usually a page refresh). + * Note: the prepended notification may not be in the correct order. This is probably ok. + * + * This function: + * 1. Gets the archived notification's data from the cache using its ID + * 2. If the archive list exists in the cache, adds the notification to the beginning of that list + * 3. Returns the original mutation result + * + * @param _ - Existing cache value (unused) + * @param incoming - Result (i.e. the archived notification) from the server after archiving + * @param cache - Apollo cache instance + * @param args - Mutation arguments containing the notification ID + * @returns The incoming result to be cached + */ + merge(_, incoming, { cache, args }) { + if (!args?.id) return incoming; + const id = cache.identify({ id: args.id, __typename: NotificationCacheType }); + if (!id) return incoming; + + const notification = cache.readFragment({ id, fragment: NOTIFICATION_FRAGMENT }); + if (!notification) return incoming; + + cache.updateQuery( + { + query: getNotifications, + // @ts-expect-error the cache only uses the filter type; the limit & offset are superfluous. + variables: { filter: { type: NotificationType.Archive } }, + }, + (data) => { + // no data means the archive hasn't been queried yet, in which case this operation is unnecessary + if (!data) return; + const updated = structuredClone(data); + updated.notifications.list.unshift(notification); + return updated; + } + ); + + return incoming; + }, + }, deleteNotification: { /** * Ensures that a deleted notification is removed from the cache + @@ -119,10 +169,7 @@ const defaultCacheConfig: InMemoryCacheConfig = { */ merge(_, incoming, { cache, args }) { if (args?.id) { - const id = cache.identify({ - id: args.id, - __typename: NotificationType, - }); + const id = cache.identify({ id: args.id, __typename: NotificationCacheType }); cache.evict({ id }); } // Removes references to evicted notification, preventing dangling references From 4adf354b8d47ceb8c5625332cf08a92f86feaae1 Mon Sep 17 00:00:00 2001 From: Pujit Mehrotra Date: Wed, 11 Dec 2024 15:25:39 -0500 Subject: [PATCH 2/2] refactor(web): evict notification list on archival instead of manually modifying cache --- web/helpers/apollo-cache/index.ts | 66 +++++++++++++++---------------- 1 file changed, 31 insertions(+), 35 deletions(-) diff --git a/web/helpers/apollo-cache/index.ts b/web/helpers/apollo-cache/index.ts index 7e918b6737..588b41c791 100644 --- a/web/helpers/apollo-cache/index.ts +++ b/web/helpers/apollo-cache/index.ts @@ -1,10 +1,7 @@ import { InMemoryCache, type InMemoryCacheConfig } from '@apollo/client/core/index.js'; -import { - getNotifications, - NOTIFICATION_FRAGMENT, -} from '~/components/Notifications/graphql/notification.query'; +import { getNotifications } from '~/components/Notifications/graphql/notification.query'; import { NotificationType, type NotificationOverview } from '~/composables/gql/graphql'; -import { NotificationType as NotificationCacheType } from '../../composables/gql/typename'; +import { NotificationType as NotificationCacheType } from '~/composables/gql/typename'; import { mergeAndDedup } from './merge'; /**------------------------------------------------------------------------ @@ -112,47 +109,46 @@ const defaultCacheConfig: InMemoryCacheConfig = { }, archiveNotification: { /** - * Ensures newly archived notifications appear in the archive list immediately. + * Ensures newly archived notifications appear in the archive list without a page reload. * - * When a notification is archived, we need to manually prepend it to the archive list - * in the cache if that list has already been queried. Otherwise, the archived notification - * won't appear until the next refetch (usually a page refresh). - * Note: the prepended notification may not be in the correct order. This is probably ok. + * When a notification is archived, we need to evict the cached archive list to force a refetch. + * This ensures the archived notification appears in the correct sorted position. + * + * If the archive list is empty, we evict the entire notifications cache since evicting an empty + * list has no effect. This forces a full refetch of all notification data. + * Note: This may cause temporary jitter with infinite scroll. * * This function: - * 1. Gets the archived notification's data from the cache using its ID - * 2. If the archive list exists in the cache, adds the notification to the beginning of that list - * 3. Returns the original mutation result + * 1. Checks if the cache has an archive list. If not, this function is a no-op. + * 2. If the list has items, evicts just the archive list + * 3. If it is empty, evicts the entire notifications cache + * 4. Runs garbage collection to clean up orphaned references + * 5. Returns the original mutation result * * @param _ - Existing cache value (unused) * @param incoming - Result (i.e. the archived notification) from the server after archiving * @param cache - Apollo cache instance - * @param args - Mutation arguments containing the notification ID * @returns The incoming result to be cached */ - merge(_, incoming, { cache, args }) { - if (!args?.id) return incoming; - const id = cache.identify({ id: args.id, __typename: NotificationCacheType }); - if (!id) return incoming; - - const notification = cache.readFragment({ id, fragment: NOTIFICATION_FRAGMENT }); - if (!notification) return incoming; + merge(_, incoming, { cache }) { + const archiveQuery = cache.readQuery({ + query: getNotifications, + // @ts-expect-error the cache only uses the filter type; the limit & offset are superfluous. + variables: { filter: { type: NotificationType.Archive } }, + }); + if (!archiveQuery) return incoming; - cache.updateQuery( - { - query: getNotifications, - // @ts-expect-error the cache only uses the filter type; the limit & offset are superfluous. - variables: { filter: { type: NotificationType.Archive } }, - }, - (data) => { - // no data means the archive hasn't been queried yet, in which case this operation is unnecessary - if (!data) return; - const updated = structuredClone(data); - updated.notifications.list.unshift(notification); - return updated; - } - ); + if (archiveQuery.notifications.list.length === 0) { + cache.evict({ fieldName: 'notifications' }); + } else { + cache.evict({ + id: archiveQuery.notifications.id, + fieldName: 'list', + args: { filter: { type: NotificationType.Archive } }, + }); + } + cache.gc(); return incoming; }, },