diff --git a/shared/chat/conversation/team-hooks.tsx b/shared/chat/conversation/team-hooks.tsx index 94d264f5709b..548e2ce2afa7 100644 --- a/shared/chat/conversation/team-hooks.tsx +++ b/shared/chat/conversation/team-hooks.tsx @@ -1,20 +1,19 @@ import * as C from '@/constants' import * as T from '@/constants/types' -import {useEngineActionListener} from '@/engine/action-listener' import {useCurrentUserState} from '@/stores/current-user' import {useUsersState} from '@/stores/users' import * as Teams from '@/constants/teams' import logger from '@/logger' import * as React from 'react' import {useTeamsListMap, useTeamsRoleMap} from '@/teams/use-teams-list' +import type * as EngineGen from '@/constants/rpc' import { - type CachedResourceCache, - getCachedResourceCache, + type CachedResourceInvalidation, + createCachedResourceNamespace, useCachedResource, } from '@/util/use-cached-resource' import {updateChosenChannelsTeamnames, useChosenChannelsTeamnames} from './manage-channels-badge' import {useThreadMeta} from './thread-context' -import {registerExternalResetter} from '@/util/zustand' type ChatTeamState = { role: T.Teams.MaybeTeamRoleType @@ -43,30 +42,38 @@ export type ChatManageChannelsBadge = ChatManageChannelsBadgeState & { } type ChatTeamMembersData = ReadonlyMap -type TeamCacheKey = T.Teams.TeamID | undefined -type TeamCacheMap = Map> const emptyChatTeamMembersData: ChatTeamMembersData = new Map() // Module level so switching conversations (or channels within a team) reuses // loaded members instead of refetching. teamChangedByID & friends invalidate. -const chatTeamMembersCacheMap: TeamCacheMap = new Map() +const chatTeamMembers = createCachedResourceNamespace( + 'chat-team-hooks-caches', + () => emptyChatTeamMembersData +) const chatTeamReloadStaleMs = 5 * 60_000 -// module scope outlives sign-out, so the next user would be served the previous -// user's member lists -registerExternalResetter('chat-team-hooks-caches', () => { - chatTeamMembersCacheMap.clear() -}) - -// A disabled "shadow" instance (one that returns the context value instead of -// its own) must NOT share the loader's cache: with enabled=false -// useCachedResource resets the cache, which would clobber the loader's data. -// Give shadows a private throwaway map so their resets are harmless. -const useTeamCacheMap = (sharedCacheMap: TeamCacheMap, forceLocalCache: boolean) => { - const [localCacheMap] = React.useState>(() => new Map()) - return forceLocalCache ? localCacheMap : sharedCacheMap -} +const teamMemberInvalidations = (teamID?: T.Teams.TeamID) => + [ + { + type: 'keybase.1.NotifyTeam.teamChangedByID', + when: (action: EngineGen.Actions) => + (action as EngineGen.ActionOf<'keybase.1.NotifyTeam.teamChangedByID'>).payload.params.teamID === + teamID, + }, + { + effect: 'clear', + type: 'keybase.1.NotifyTeam.teamDeleted', + when: (action: EngineGen.Actions) => + (action as EngineGen.ActionOf<'keybase.1.NotifyTeam.teamDeleted'>).payload.params.teamID === teamID, + }, + { + effect: 'clear', + type: 'keybase.1.NotifyTeam.teamExit', + when: (action: EngineGen.Actions) => + (action as EngineGen.ActionOf<'keybase.1.NotifyTeam.teamExit'>).payload.params.teamID === teamID, + }, + ] satisfies ReadonlyArray const loadableTeamID = (teamID: T.Teams.TeamID) => teamID && teamID !== T.Teams.noTeamID && teamID !== T.Teams.newTeamWizardTeamID ? teamID : undefined @@ -104,24 +111,13 @@ const useChatTeamRaw = (teamID: T.Teams.TeamID, teamname?: string): ChatTeam => } } -const useChatTeamMembersRaw = ( - teamID: T.Teams.TeamID, - enabled = true, - subscribeToUpdates = enabled, - forceLocalCache = false -): ChatTeamMembers => { +const useChatTeamMembersRaw = (teamID: T.Teams.TeamID, enabled = true): ChatTeamMembers => { const validTeamID = loadableTeamID(teamID) - const cacheMap = useTeamCacheMap(chatTeamMembersCacheMap, forceLocalCache) - const cache = React.useMemo( - () => getCachedResourceCache(cacheMap, emptyChatTeamMembersData, validTeamID), - [cacheMap, validTeamID] - ) - - const {clear, data, loaded, loading, reload} = useCachedResource({ - cache, + const {data, loaded, loading, reload} = useCachedResource({ cacheKey: validTeamID, - enabled: enabled && !!validTeamID, + enabled, initialData: emptyChatTeamMembersData, + invalidateOn: teamMemberInvalidations(validTeamID), load: async () => { const members = Teams.rpcDetailsToMemberInfos( (await T.RPCGen.teamsTeamGetMembersByIDRpcPromise({id: validTeamID ?? T.Teams.noTeamID})) ?? [] @@ -134,40 +130,13 @@ const useChatTeamMembersRaw = ( ) return members }, + namespace: chatTeamMembers, onError: error => { logger.warn(`Failed to reload chat team members for ${validTeamID}`, error) }, staleMs: chatTeamReloadStaleMs, }) - useEngineActionListener( - 'keybase.1.NotifyTeam.teamChangedByID', - action => { - if (action.payload.params.teamID === validTeamID) { - void reload() - } - }, - subscribeToUpdates - ) - useEngineActionListener( - 'keybase.1.NotifyTeam.teamDeleted', - action => { - if (action.payload.params.teamID === validTeamID) { - clear(validTeamID) - } - }, - subscribeToUpdates - ) - useEngineActionListener( - 'keybase.1.NotifyTeam.teamExit', - action => { - if (action.payload.params.teamID === validTeamID) { - clear(validTeamID) - } - }, - subscribeToUpdates - ) - // `loading` means "nothing to show yet" - a background revalidation of cached // data must not flip callers back to their empty/spinner state. return {loading: loading && !loaded, members: data, reload} @@ -195,12 +164,7 @@ export const ChatTeamProvider = (props: React.PropsWithChildren) => { const enabled = teamType !== 'adhoc' && !!loadableTeamID(teamID) const sameAsOuter = outer?.teamID === teamID const team = useChatTeamRaw(teamID, teamname) - const members = useChatTeamMembersRaw( - teamID, - enabled && !sameAsOuter, - enabled && !sameAsOuter, - sameAsOuter - ) + const members = useChatTeamMembersRaw(teamID, enabled && !sameAsOuter) const value: ChatTeamContextValue = sameAsOuter ? outer! : {members, team, teamID} return {children} } @@ -215,7 +179,7 @@ export const useChatTeam = (teamID: T.Teams.TeamID, teamname?: string): ChatTeam export const useChatTeamMembers = (teamID: T.Teams.TeamID): ChatTeamMembers => { const context = React.useContext(ChatTeamContext) const useContextValue = context?.teamID === teamID - const raw = useChatTeamMembersRaw(teamID, !useContextValue, !useContextValue, useContextValue) + const raw = useChatTeamMembersRaw(teamID, !useContextValue) return useContextValue ? context.members : raw } diff --git a/shared/chat/user-emoji.tsx b/shared/chat/user-emoji.tsx index 96f39ae450f6..2e247c2c673c 100644 --- a/shared/chat/user-emoji.tsx +++ b/shared/chat/user-emoji.tsx @@ -1,13 +1,7 @@ import * as React from 'react' import * as T from '@/constants/types' import {useEmojiState} from '@/teams/emojis/use-emoji' -import { - type CachedResourceCache, - createCachedResourceCache, - getCachedResourceCache, - useCachedResource, -} from '@/util/use-cached-resource' -import {registerExternalResetter} from '@/util/zustand' +import {createCachedResourceNamespace, useCachedResource} from '@/util/use-cached-resource' const emptyEmojiGroups: ReadonlyArray = [] const emptyEmojis: ReadonlyArray = [] @@ -23,18 +17,15 @@ const emptyUserEmojiData: UserEmojiData = {emojiGroups: emptyEmojiGroups, emojis // the service side - it resolves two attachment URLs per custom emoji - so the // suggestor remounting on each ':' trigger used to refetch the entire set. The // shared cache also collapses concurrent mounts onto a single in-flight request. -const userEmojiCaches = new Map>() -const userEmojiStaleMs = 60_000 // an entry holds every custom emoji the conv can see, with two resolved // attachment URLs each, and every channel of a team keeps its own copy, so this // is capped by size rather than left to grow for the life of the session -const userEmojiCacheMax = 32 - -// module scope outlives sign-out, so the next user would be served the previous -// user's custom emoji until the entries went stale -registerExternalResetter('chat-user-emoji-caches', () => { - userEmojiCaches.clear() -}) +const userEmoji = createCachedResourceNamespace( + 'chat-user-emoji-caches', + () => emptyUserEmojiData, + {maxEntries: 32} +) +const userEmojiStaleMs = 60_000 // Adding, aliasing or removing an emoji has to drop the shared entries from the // store, not from a mounted consumer: the edit happens on the team emoji page, @@ -45,7 +36,7 @@ useEmojiState.subscribe((state, prev) => { if (state.emojiUpdatedTrigger === prev.emojiUpdatedTrigger) { return } - userEmojiCaches.forEach((cache, key) => cache.invalidate(key)) + userEmoji.invalidateAll() }) const flattenUserEmojis = (groups: ReadonlyArray) => { @@ -72,36 +63,6 @@ export const useUserEmoji = ({ // the store subscription above already dropped the shared entries; this makes // the mounted consumers re-run their load and pick the new set up const emojiUpdatedTrigger = useEmojiState(s => s.emojiUpdatedTrigger) - // a disabled instance resets the cache it holds, so keep those off the shared one - const [localCache] = React.useState>(() => - createCachedResourceCache(emptyUserEmojiData, requestKey) - ) - // a disabled instance must not seed the shared map: it never loads, so the - // entry it created would sit there empty for the life of the session - const sharedCache = React.useMemo(() => { - if (disabled) { - return undefined - } - // drop the least recently asked for entries rather than the whole map: a - // wholesale clear at the cap makes the conversation you switch back to - // re-issue localUserEmojis even though its entry was still fresh. Map - // iterates in insertion order, and re-inserting on use below keeps that - // order meaningful. - if (userEmojiCaches.has(requestKey)) { - const existing = userEmojiCaches.get(requestKey)! - userEmojiCaches.delete(requestKey) - userEmojiCaches.set(requestKey, existing) - } else { - while (userEmojiCaches.size >= userEmojiCacheMax) { - const oldest = userEmojiCaches.keys().next() - if (oldest.done) { - break - } - userEmojiCaches.delete(oldest.value) - } - } - return getCachedResourceCache(userEmojiCaches, emptyUserEmojiData, requestKey) - }, [disabled, requestKey]) const load = React.useCallback(async () => { const results = await T.RPCChat.localUserEmojisRpcPromise({ convID: @@ -119,11 +80,11 @@ export const useUserEmoji = ({ }, [conversationIDKey, requestOnlyInTeam]) const {data, loading} = useCachedResource({ - cache: sharedCache ?? localCache, cacheKey: requestKey, enabled: !disabled, initialData: emptyUserEmojiData, load, + namespace: userEmoji, refreshKey: emojiUpdatedTrigger, staleMs: userEmojiStaleMs, }) diff --git a/shared/teams/common/activity.tsx b/shared/teams/common/activity.tsx index 5473a08eb6f8..891883907aed 100644 --- a/shared/teams/common/activity.tsx +++ b/shared/teams/common/activity.tsx @@ -2,7 +2,7 @@ import * as React from 'react' import * as Kb from '@/common-adapters' import * as T from '@/constants/types' import logger from '@/logger' -import {createCachedResourceCache, type CachedResourceCache, useCachedResource} from '@/util/use-cached-resource' +import {createCachedResourceNamespace, useCachedResource} from '@/util/use-cached-resource' const activityToIcon: {[key in 'active' | 'recently']: Kb.IconType} = { active: 'iconfont-campfire-burning', @@ -76,16 +76,23 @@ const parseActivityLevels = ( } } -const useActivityLevelsRaw = ( - cache: CachedResourceCache, - enabled = true -): ActivityLevels => { +// One entry for the whole app rather than one per provider: the teams root, a +// team, a channel and the add-to-channels modal nest, and each mount used to pay +// its own getLastActiveForTeams. The trade is that a remount inside the stale +// window is served from the entry instead of refetching - activity levels are a +// coarse "how busy is this" bucket, so up to staleMs of drift is acceptable. +const activityLevels = createCachedResourceNamespace( + 'teams-activity-levels', + () => emptyActivityLevelsData +) + +const useActivityLevelsRaw = (enabled = true): ActivityLevels => { const {data, loaded, loading, reload} = useCachedResource({ - cache, cacheKey: activityLevelsCacheKey, enabled, initialData: emptyActivityLevelsData, load: async () => parseActivityLevels(await T.RPCChat.localGetLastActiveForTeamsRpcPromise()), + namespace: activityLevels, onError: error => { logger.warn('Failed to load activity levels', error) }, @@ -97,13 +104,7 @@ const useActivityLevelsRaw = ( export const ActivityLevelsProvider = (props: React.PropsWithChildren) => { const {children} = props - const [cache] = React.useState(() => - createCachedResourceCache( - emptyActivityLevelsData, - activityLevelsCacheKey - ) - ) - const value = useActivityLevelsRaw(cache) + const value = useActivityLevelsRaw() return {children} } diff --git a/shared/teams/common/general-conv.tsx b/shared/teams/common/general-conv.tsx index 6fe654c64e2e..687842c1b3e2 100644 --- a/shared/teams/common/general-conv.tsx +++ b/shared/teams/common/general-conv.tsx @@ -1,14 +1,7 @@ -import * as React from 'react' import * as T from '@/constants/types' import * as Meta from '@/constants/chat/meta' import {metasReceived} from '@/chat/inbox/metadata' -import {registerExternalResetter} from '@/util/zustand' -import { - type CachedResourceCache, - createCachedResourceCache, - getCachedResourceCache, - useCachedResource, -} from '@/util/use-cached-resource' +import {createCachedResourceNamespace, useCachedResource} from '@/util/use-cached-resource' type GeneralConvData = T.Chat.ConversationIDKey | undefined @@ -18,35 +11,17 @@ const noGeneralConv: GeneralConvData = undefined // team rows and the bot install modal - and each used to hold the answer in its // own state, so every mount was another findGeneralConvFromTeamID. Share one // cache per team, and let it live a while since the answer is effectively static. -const generalConvCaches = new Map>() +const generalConvs = createCachedResourceNamespace( + 'teams-general-conv-caches', + () => noGeneralConv +) const generalConvStaleMs = 5 * 60_000 -// module scope outlives sign-out. Dropping the map is not enough on its own: a -// consumer that is still mounted through the sign-out holds the cache object -// itself, so each one has to be emptied as well. -registerExternalResetter('teams-general-conv-caches', () => { - generalConvCaches.forEach((cache, teamID) => cache.reset(noGeneralConv, teamID)) - generalConvCaches.clear() -}) - export const useGeneralConvIDKey = (teamID?: T.Teams.TeamID, enabled = true) => { const validTeamID = teamID && teamID !== T.Teams.noTeamID ? teamID : undefined - const on = enabled && !!validTeamID - const cacheKey = validTeamID ?? T.Teams.noTeamID - // a disabled instance resets whatever cache it holds, so keep it off the shared one - const [localCache] = React.useState>(() => - createCachedResourceCache(noGeneralConv, cacheKey) - ) - // an off instance must not seed the shared map: it never loads, so the entry it - // created would sit there empty for the life of the session - const sharedCache = React.useMemo( - () => (on ? getCachedResourceCache(generalConvCaches, noGeneralConv, cacheKey) : undefined), - [cacheKey, on] - ) const {data} = useCachedResource({ - cache: sharedCache ?? localCache, - cacheKey, - enabled: on, + cacheKey: validTeamID, + enabled, initialData: noGeneralConv, load: async () => { const conv = await T.RPCChat.localFindGeneralConvFromTeamIDRpcPromise({ @@ -59,6 +34,7 @@ export const useGeneralConvIDKey = (teamID?: T.Teams.TeamID, enabled = true) => metasReceived([meta]) return meta.conversationIDKey }, + namespace: generalConvs, staleMs: generalConvStaleMs, }) return data diff --git a/shared/teams/common/use-loaded-team-channels.tsx b/shared/teams/common/use-loaded-team-channels.tsx index 8755f7c95226..778d454a3c38 100644 --- a/shared/teams/common/use-loaded-team-channels.tsx +++ b/shared/teams/common/use-loaded-team-channels.tsx @@ -1,15 +1,17 @@ import * as C from '@/constants' import * as Chat from '@/constants/chat' import * as T from '@/constants/types' -import {useEngineActionListener} from '@/engine/action-listener' import isEqual from 'lodash/isEqual' import logger from '@/logger' import * as React from 'react' -import {nextReloadEpoch} from '@/util/reload-epoch' -import {registerExternalResetter} from '@/util/zustand' +import type * as EngineGen from '@/constants/rpc' import {registerTeamChannelsInvalidator} from './team-channels-invalidation' import {useLoadedTeam} from '../team/use-loaded-team' -import {type CachedResourceCache, getCachedResourceCache, useCachedResource} from '@/util/use-cached-resource' +import { + type CachedResourceInvalidation, + createCachedResourceNamespace, + useCachedResource, +} from '@/util/use-cached-resource' type LoadedTeamChannels = { channels: ReadonlyMap @@ -32,11 +34,6 @@ type LoadedTeamChannelsData = Pick< LoadedTeamChannels, 'channels' | 'channelMetas' | 'channelParticipants' > -type LoadedTeamChannelsCacheMap = Map< - T.Teams.TeamID | undefined, - CachedResourceCache -> - const LoadedTeamChannelsContext = React.createContext(null) const loadedTeamChannelsReloadStaleMs = 5_000 @@ -65,6 +62,24 @@ const recycleMap = (old: ReadonlyMap, next: Map): ReadonlyMap< return unchanged ? old : next } +// nothing moved at all: hand back the very object the cache already holds, so +// useCachedResource settles without a state change +const recycleChannels = ( + previous: LoadedTeamChannelsData, + next: LoadedTeamChannelsData +): LoadedTeamChannelsData => { + const recycled = { + channelMetas: recycleMap(previous.channelMetas, new Map(next.channelMetas)), + channelParticipants: recycleMap(previous.channelParticipants, new Map(next.channelParticipants)), + channels: recycleMap(previous.channels, new Map(next.channels)), + } + return recycled.channelMetas === previous.channelMetas && + recycled.channelParticipants === previous.channelParticipants && + recycled.channels === previous.channels + ? previous + : recycled +} + const loadableTeamID = (teamID: T.Teams.TeamID) => teamID && teamID !== T.Teams.noTeamID && teamID !== T.Teams.newTeamWizardTeamID ? teamID : undefined @@ -74,44 +89,26 @@ const emptyLoadedTeamChannelsData: LoadedTeamChannelsData = { channels: emptyChannels, } -// One map for every consumer. The stale window and the single-flight both live on -// the cache object, so callers holding separate maps cannot see each other's -// in-flight request and each issue their own getTLFConversationsLocal - which -// localizes every channel in the team. Measured at 7 calls for one team inside -// 1.5s before this was shared. -const loadedTeamChannelsCache: LoadedTeamChannelsCacheMap = new Map() -const loadedTeamChannelsInvalidationListeners = new Set< - (teamID: T.Teams.TeamID | undefined, epoch: number) => void ->() - -// module scope outlives sign-out and this is per-user team data -registerExternalResetter('loaded-team-channels-cache', () => { - loadedTeamChannelsCache.forEach(cache => cache.reset(emptyLoadedTeamChannelsData, undefined)) - loadedTeamChannelsCache.clear() -}) - -// While every consumer held a private cache a remount happened to refetch, which -// is what the channel list relied on after a create. Sharing one cache means a -// remount inside the stale window serves the pre-change channels instead, so the -// create/delete screens have to drop this explicitly. +// One entry per team. The stale window and the single-flight both live on the +// entry, so consumers holding separate ones cannot see each other's in-flight +// request and each issue their own getTLFConversationsLocal - which localizes +// every channel in the team. Measured at 7 calls for one team inside 1.5s before +// this was shared. +const loadedTeamChannels = createCachedResourceNamespace( + 'loaded-team-channels-cache', + () => emptyLoadedTeamChannelsData +) + +// Creating or deleting a channel fires no teamChangedByID, and a remount inside +// the stale window would serve the pre-change channels, so the create/delete +// screens drop the entry explicitly. registerTeamChannelsInvalidator((teamID: T.Teams.TeamID) => { const key = loadableTeamID(teamID) - loadedTeamChannelsCache.get(key)?.invalidate(key) - // one invalidation is one event: every listener reloads against the same - // epoch so they share an rpc instead of superseding each other - const epoch = nextReloadEpoch() - loadedTeamChannelsInvalidationListeners.forEach(listener => listener(key, epoch)) + if (key) { + loadedTeamChannels.invalidate(key) + } }) -// forceLocalCache: a disabled "shadow" instance (one that returns the context -// value instead of its own) must NOT share the loader's cache map. With enabled=false -// useCachedResource resets the cache (loadedAt=0), which would clobber the loader's -// loaded data. Give shadows a private throwaway map so their resets are harmless. -const useLoadedTeamChannelsCacheMap = (forceLocalCache: boolean) => { - const [localCacheMap] = React.useState(() => new Map()) - return forceLocalCache ? localCacheMap : loadedTeamChannelsCache -} - export const teamChannelsRPCParams = (teamname: string) => ({ membersType: T.RPCChat.ConversationMembersType.team, tlfName: teamname, @@ -120,55 +117,43 @@ export const teamChannelsRPCParams = (teamname: string) => ({ // keep a team's channel list fresh: reload on team changes, drop it when the // team is deleted or left -export const useReloadOnTeamChannelChanges = ( - teamID: T.Teams.TeamID | undefined, - enabled: boolean, - reload: () => unknown, - clear: () => void -) => { - useEngineActionListener('keybase.1.NotifyTeam.teamChangedByID', action => { - if (enabled && action.payload.params.teamID === teamID) { - void reload() - } - }) - useEngineActionListener('keybase.1.NotifyTeam.teamDeleted', action => { - if (enabled && action.payload.params.teamID === teamID) { - clear() - } - }) - useEngineActionListener('keybase.1.NotifyTeam.teamExit', action => { - if (enabled && action.payload.params.teamID === teamID) { - clear() - } - }) -} +const teamChannelInvalidations = (teamID: T.Teams.TeamID | undefined) => + [ + { + type: 'keybase.1.NotifyTeam.teamChangedByID', + when: (action: EngineGen.Actions) => + (action as EngineGen.ActionOf<'keybase.1.NotifyTeam.teamChangedByID'>).payload.params.teamID === + teamID, + }, + { + effect: 'clear', + type: 'keybase.1.NotifyTeam.teamDeleted', + when: (action: EngineGen.Actions) => + (action as EngineGen.ActionOf<'keybase.1.NotifyTeam.teamDeleted'>).payload.params.teamID === teamID, + }, + { + effect: 'clear', + type: 'keybase.1.NotifyTeam.teamExit', + when: (action: EngineGen.Actions) => + (action as EngineGen.ActionOf<'keybase.1.NotifyTeam.teamExit'>).payload.params.teamID === teamID, + }, + ] satisfies ReadonlyArray const useLoadedTeamChannelsRaw = ( teamID: T.Teams.TeamID, providedTeamname?: string, - enabled = true, - forceLocalCache = false + enabled = true ): LoadedTeamChannels => { const validTeamID = loadableTeamID(teamID) const { teamMeta: {teamname: loadedTeamname}, } = useLoadedTeam(teamID, enabled) const teamnameToLoad = providedTeamname || loadedTeamname - // useCachedResource resets whatever cache it holds while disabled, so a - // disabled instance must never hold the shared one — including the ordinary - // consumer whose teamname has not resolved yet, which would otherwise wipe a - // real loader's data mid-flight. Gate the cache on exactly the load condition. - const canLoad = enabled && !!validTeamID && !!teamnameToLoad - const cacheMap = useLoadedTeamChannelsCacheMap(forceLocalCache || !canLoad) - const cache = React.useMemo( - () => getCachedResourceCache(cacheMap, emptyLoadedTeamChannelsData, validTeamID), - [cacheMap, validTeamID] - ) - const {data, loading, reload, clear} = useCachedResource({ - cache, + const {data, loading, reload} = useCachedResource({ cacheKey: validTeamID, - enabled: canLoad, + enabled: enabled && !!teamnameToLoad, initialData: emptyLoadedTeamChannelsData, + invalidateOn: teamChannelInvalidations(validTeamID), load: async () => { if (!teamnameToLoad) { return emptyLoadedTeamChannelsData @@ -199,45 +184,17 @@ const useLoadedTeamChannelsRaw = ( } } - const previous = cache.getData() - const recycled = { - channelMetas: recycleMap(previous.channelMetas, channelMetas), - channelParticipants: recycleMap(previous.channelParticipants, channelParticipants), - channels: recycleMap(previous.channels, channels), - } - // nothing moved at all: hand back the very object the cache already holds, - // so useCachedResource settles without a state change - return recycled.channelMetas === previous.channelMetas && - recycled.channelParticipants === previous.channelParticipants && - recycled.channels === previous.channels - ? previous - : recycled + return {channelMetas, channelParticipants, channels} }, + namespace: loadedTeamChannels, onError: error => { logger.warn(`Failed to load team channels for ${validTeamID}`, error) }, + recycle: recycleChannels, refreshKey: teamnameToLoad, staleMs: loadedTeamChannelsReloadStaleMs, }) - useReloadOnTeamChannelChanges(validTeamID, enabled, reload, () => clear(validTeamID)) - - // a mounted list must pick up an invalidation too, not just the next mount - React.useEffect(() => { - if (!enabled || !validTeamID) { - return - } - const listener = (invalidatedTeamID: T.Teams.TeamID | undefined, epoch: number) => { - if (invalidatedTeamID === validTeamID) { - void reload(epoch) - } - } - loadedTeamChannelsInvalidationListeners.add(listener) - return () => { - loadedTeamChannelsInvalidationListeners.delete(listener) - } - }, [enabled, validTeamID, reload]) - const {channelMetas, channelParticipants, channels} = data return React.useMemo( () => ({channelMetas, channelParticipants, channels, loading, reload}), @@ -249,11 +206,8 @@ export const LoadedTeamChannelsProvider = ( props: React.PropsWithChildren<{teamID: T.Teams.TeamID; teamname?: string}> ) => { const {children, teamID, teamname} = props - const loadedTeamChannels = useLoadedTeamChannelsRaw(teamID, teamname, true) - const value = React.useMemo( - () => ({...loadedTeamChannels, teamID}), - [loadedTeamChannels, teamID] - ) + const channels = useLoadedTeamChannelsRaw(teamID, teamname, true) + const value = React.useMemo(() => ({...channels, teamID}), [channels, teamID]) return {children} } @@ -266,7 +220,7 @@ export const useLoadedTeamChannels = ( // a disabled consumer still reads a provider's already-loaded value when there // is one - it only must not issue a load of its own const useContextValue = context?.teamID === teamID - const raw = useLoadedTeamChannelsRaw(teamID, teamname, enabled && !useContextValue, useContextValue) + const raw = useLoadedTeamChannelsRaw(teamID, teamname, enabled && !useContextValue) return useContextValue ? context : raw } diff --git a/shared/teams/team/settings-tab/default-channels.tsx b/shared/teams/team/settings-tab/default-channels.tsx index 6604d8f4dcc4..a641ba827f18 100644 --- a/shared/teams/team/settings-tab/default-channels.tsx +++ b/shared/teams/team/settings-tab/default-channels.tsx @@ -3,10 +3,9 @@ import * as React from 'react' import * as Kb from '@/common-adapters' import * as T from '@/constants/types' import logger from '@/logger' -import {registerExternalResetter} from '@/util/zustand' import {ChannelsWidget} from '@/teams/common' import {useLoadedTeam} from '../use-loaded-team' -import {type CachedResourceCache, getCachedResourceCache, useCachedResource} from '@/util/use-cached-resource' +import {createCachedResourceNamespace, useCachedResource} from '@/util/use-cached-resource' type Props = { teamID: T.Teams.TeamID @@ -20,15 +19,10 @@ const emptyDefaultChannels: DefaultChannelsData = [] // One cache per team, shared by every consumer: each load is a remote // chat.1.remote.getDefaultTeamChannels round trip, so a per-instance cache turns // every extra mount of the settings tab into another hit on the chat rate limit. -const defaultChannelsCaches = new Map< - T.Teams.TeamID, - CachedResourceCache ->() - -// module scope outlives sign-out, so the next user would inherit this user's channels -registerExternalResetter('teams-default-channels-caches', () => { - defaultChannelsCaches.clear() -}) +const defaultChannelsResource = createCachedResourceNamespace( + 'teams-default-channels-caches', + () => emptyDefaultChannels +) // resolve rather than reject on failure: consumers render an empty list (and no // spinner) on error, and a cached failure keeps a broken team from re-requesting @@ -48,15 +42,11 @@ const loadDefaultChannels = async (teamID: T.Teams.TeamID): Promise { - const cache = React.useMemo( - () => getCachedResourceCache(defaultChannelsCaches, emptyDefaultChannels, teamID), - [teamID] - ) const {data, loaded, loading, reload} = useCachedResource({ - cache, - cacheKey: teamID, + cacheKey: teamID || undefined, initialData: emptyDefaultChannels, load: async () => await loadDefaultChannels(teamID), + namespace: defaultChannelsResource, staleMs: defaultChannelsStaleMs, }) diff --git a/shared/teams/team/settings-tab/retention/index.tsx b/shared/teams/team/settings-tab/retention/index.tsx index 6d4ed7cb38f5..9a15905513fa 100644 --- a/shared/teams/team/settings-tab/retention/index.tsx +++ b/shared/teams/team/settings-tab/retention/index.tsx @@ -6,14 +6,8 @@ import * as Kb from '@/common-adapters' import * as T from '@/constants/types' import SaveIndicator from '@/common-adapters/save-indicator' import logger from '@/logger' -import {registerExternalResetter} from '@/util/zustand' -import {useEngineActionListener} from '@/engine/action-listener' -import { - type CachedResourceCache, - createCachedResourceCache, - getCachedResourceCache, - useCachedResource, -} from '@/util/use-cached-resource' +import type * as EngineGen from '@/constants/rpc' +import {createCachedResourceNamespace, useCachedResource} from '@/util/use-cached-resource' import {useLoadedTeam} from '../../use-loaded-team' import {useConfirm} from './use-confirm' import {ConversationThreadProvider, useThreadMeta} from '@/chat/conversation/thread-context' @@ -380,30 +374,28 @@ const noTeamRetentionPolicy: TeamRetentionData = undefined // One cache per team, shared by every consumer (settings tab and every chat info // panel for the team): a burst of remounts would otherwise be a burst of // GetTeamRetentionLocal calls with identical arguments. -const teamRetentionCaches = new Map>() - -// module scope outlives sign-out, so the next user would inherit this user's policies -registerExternalResetter('teams-retention-caches', () => { - teamRetentionCaches.clear() -}) +const teamRetentionPolicies = createCachedResourceNamespace( + 'teams-retention-caches', + () => noTeamRetentionPolicy +) const useLoadedTeamRetentionPolicy = (teamID: T.Teams.TeamID) => { - const enabled = !!teamID && teamID !== T.Teams.noTeamID - // an adhoc conversation has no team, and useCachedResource resets the cache it - // holds when disabled — give those instances their own throwaway cache so they - // can't wipe the shared one out from under a real loader - const [localCache] = React.useState>(() => - createCachedResourceCache(noTeamRetentionPolicy, teamID) - ) - const sharedCache = React.useMemo( - () => getCachedResourceCache(teamRetentionCaches, noTeamRetentionPolicy, teamID), - [teamID] - ) - const {data: teamPolicy, reload} = useCachedResource({ - cache: enabled ? sharedCache : localCache, - cacheKey: teamID, - enabled, + // an adhoc conversation has no team + const validTeamID = teamID && teamID !== T.Teams.noTeamID ? teamID : undefined + const {data: teamPolicy} = useCachedResource({ + cacheKey: validTeamID, initialData: noTeamRetentionPolicy, + // the notification carries the new policy, but a reload is what refreshes + // the shared entry for the other consumers too + invalidateOn: [ + { + type: 'chat.1.NotifyChat.ChatSetTeamRetention', + when: (action: EngineGen.Actions) => + (action as EngineGen.ActionOf<'chat.1.NotifyChat.ChatSetTeamRetention'>).payload.params + .teamID === validTeamID, + }, + ], + namespace: teamRetentionPolicies, // resolve rather than reject on failure: the picker falls back to the default // "retain" policy on error instead of spinning forever load: async () => { @@ -422,14 +414,6 @@ const useLoadedTeamRetentionPolicy = (teamID: T.Teams.TeamID) => { staleMs: teamRetentionStaleMs, }) - // the notification carries the new policy, but reload() is what actually - // invalidates the shared cache for the other consumers - useEngineActionListener('chat.1.NotifyChat.ChatSetTeamRetention', action => { - if (action.payload.params.teamID === teamID) { - void reload() - } - }) - return { loading: !teamPolicy, teamPolicy, diff --git a/shared/teams/team/use-loaded-team.test.tsx b/shared/teams/team/use-loaded-team.test.tsx index 2b48a56b72f2..c55bb7fdb80d 100644 --- a/shared/teams/team/use-loaded-team.test.tsx +++ b/shared/teams/team/use-loaded-team.test.tsx @@ -10,6 +10,7 @@ import {LoadedTeamsListProvider} from '../use-teams-list' import {LoadedTeamChannelsProvider, useLoadedTeamChannels} from '../common/use-loaded-team-channels' import {LoadedTeamProvider, useLoadedTeam} from './use-loaded-team' import {flush} from '@/test/flush' +import {notifyEngineActionListeners} from '@/engine/action-listener' import {installFakeEngine, type FakeEngine} from '@/test/fake-engine' const teamID = 'tid1' as T.Teams.TeamID @@ -139,3 +140,54 @@ test('signing out drops the shared team caches', async () => { await flush() expect(annotatedCalls()).toBe(callsWhileSignedIn + 1) }) + +// The engine listeners, the debounce and the epoch used to be hand-rolled here; +// they are now one invalidateOn declaration, so a team notification still has to +// put the team back on the wire - and exactly once for the whole screen, not +// once per mounted consumer. +test('a team change reloads the screen once', async () => { + useCurrentUserState.setState({username: 'testuser'}) + useConfigState.setState({loggedIn: true}) + render( + + + + + + ) + await flush() + expect(annotatedCalls()).toBe(1) + + act(() => { + notifyEngineActionListeners({ + payload: {params: {teamID}}, + type: 'keybase.1.NotifyTeam.teamChangedByID', + } as never) + }) + await flush() + expect(annotatedCalls()).toBe(2) +}) + +// A notification for some other team must not cost this one an rpc. +test('a change to another team leaves this one alone', async () => { + useCurrentUserState.setState({username: 'testuser'}) + useConfigState.setState({loggedIn: true}) + render( + + + + + + ) + await flush() + expect(annotatedCalls()).toBe(1) + + act(() => { + notifyEngineActionListeners({ + payload: {params: {teamID: 'tid2'}}, + type: 'keybase.1.NotifyTeam.teamChangedByID', + } as never) + }) + await flush() + expect(annotatedCalls()).toBe(1) +}) diff --git a/shared/teams/team/use-loaded-team.tsx b/shared/teams/team/use-loaded-team.tsx index e0774bbd51b8..2777e3065973 100644 --- a/shared/teams/team/use-loaded-team.tsx +++ b/shared/teams/team/use-loaded-team.tsx @@ -1,13 +1,14 @@ import * as T from '@/constants/types' -import type {DebouncedFunc} from 'lodash' -import debounce from 'lodash/debounce' -import {useEngineActionListener} from '@/engine/action-listener' import logger from '@/logger' import * as Teams from '@/constants/teams' import * as React from 'react' import {useTeamsListMap, useTeamsRoleMap} from '../use-teams-list' -import {type CachedResourceCache, getCachedResourceCache, useCachedResource} from '@/util/use-cached-resource' -import {registerExternalResetter} from '@/util/zustand' +import type * as EngineGen from '@/constants/rpc' +import { + type CachedResourceInvalidation, + createCachedResourceNamespace, + useCachedResource, +} from '@/util/use-cached-resource' type LoadedTeam = { loaded: boolean @@ -23,21 +24,36 @@ type LoadedTeamContextValue = LoadedTeam & { } type LoadedTeamData = Pick -type LoadedTeamCacheMap = Map< - T.Teams.TeamID | undefined, - CachedResourceCache -> const LoadedTeamContext = React.createContext(null) const loadedTeamReloadStaleMs = 5_000 -// One map for every consumer, for the same reason as the team channel cache: the -// stale window and the single-flight live on the cache object, so callers holding -// separate maps cannot see each other's in-flight request. While each provider -// and each provider-less consumer held its own map, 81% of getAnnotatedTeam calls -// in an e2e run landed inside their own 5s stale window - the team screen, the -// channel screen and any modal above them each paid a full 200ms team load. -const loadedTeamCache: LoadedTeamCacheMap = new Map() +// One logical change fires metadata, role map and changedByID, and a reconnect +// fires all three at once - measured as 4 getAnnotatedTeam for one team inside +// 116ms. useCachedResource coalesces them onto one reload. +const teamInvalidations = (teamID?: T.Teams.TeamID) => + [ + {type: 'keybase.1.NotifyTeam.teamMetadataUpdate'}, + {type: 'keybase.1.NotifyTeam.teamRoleMapChanged'}, + { + type: 'keybase.1.NotifyTeam.teamChangedByID', + when: (action: EngineGen.Actions) => + (action as EngineGen.ActionOf<'keybase.1.NotifyTeam.teamChangedByID'>).payload.params.teamID === + teamID, + }, + { + effect: 'clear', + type: 'keybase.1.NotifyTeam.teamDeleted', + when: (action: EngineGen.Actions) => + (action as EngineGen.ActionOf<'keybase.1.NotifyTeam.teamDeleted'>).payload.params.teamID === teamID, + }, + { + effect: 'clear', + type: 'keybase.1.NotifyTeam.teamExit', + when: (action: EngineGen.Actions) => + (action as EngineGen.ActionOf<'keybase.1.NotifyTeam.teamExit'>).payload.params.teamID === teamID, + }, + ] satisfies ReadonlyArray const loadableTeamID = (teamID: T.Teams.TeamID) => teamID && teamID !== T.Teams.noTeamID && teamID !== T.Teams.newTeamWizardTeamID ? teamID : undefined @@ -47,11 +63,16 @@ const emptyLoadedTeamData = (teamID?: T.Teams.TeamID): LoadedTeamData => ({ teamMeta: teamID ? Teams.makeTeamMeta({id: teamID}) : Teams.emptyTeamMeta, }) -// module scope outlives sign-out and this is per-user team data -registerExternalResetter('loaded-team-cache', () => { - loadedTeamCache.forEach((cache, teamID) => cache.reset(emptyLoadedTeamData(teamID), teamID)) - loadedTeamCache.clear() -}) +// One entry per team, shared by every consumer: the stale window and the +// single-flight live on the entry, so consumers holding separate ones cannot see +// each other's in-flight request. While each provider and each provider-less +// consumer held its own map, 81% of getAnnotatedTeam calls in an e2e run landed +// inside their own 5s stale window - the team screen, the channel screen and any +// modal above them each paid a full 200ms team load. +const loadedTeams = createCachedResourceNamespace( + 'loaded-team-cache', + emptyLoadedTeamData +) const roleAndDetailsFromMap = ( map: T.RPCGen.TeamRoleMapAndVersion, @@ -83,30 +104,9 @@ const annotatedTeamToMeta = ( teamname: annotatedTeam.name, }) -// forceLocalCache: a disabled "shadow" instance (one that returns the context -// value instead of its own) must NOT share the loader's cache map. With enabled=false -// useCachedResource resets the cache (loadedAt=0), which would clobber the loader's -// loaded data. Give shadows a private throwaway map so their resets are harmless. -const useLoadedTeamCacheMap = (forceLocalCache: boolean) => { - const [localCacheMap] = React.useState(() => new Map()) - return forceLocalCache ? localCacheMap : loadedTeamCache -} - -const useLoadedTeamRaw = ( - teamID: T.Teams.TeamID, - enabled = true, - subscribeToUpdates = enabled, - forceLocalCache = false -): LoadedTeam => { +const useLoadedTeamRaw = (teamID: T.Teams.TeamID, enabled = true): LoadedTeam => { const validTeamID = loadableTeamID(teamID) const {loadIfStale: loadRoleMapIfStale, roleMap} = useTeamsRoleMap() - // a disabled instance resets whatever cache it holds, so it must never hold the - // shared one - gate on exactly the load condition, not just forceLocalCache - const cacheMap = useLoadedTeamCacheMap(forceLocalCache || !enabled || !validTeamID) - const cache = React.useMemo( - () => getCachedResourceCache(cacheMap, emptyLoadedTeamData(validTeamID), validTeamID), - [cacheMap, validTeamID] - ) // Seed from the teams-list cache so the header (teamname, avatar, member count) // renders immediately instead of waiting for getAnnotatedTeam to round-trip. // key the memo on this team's meta, not on the map: the map gets a new @@ -118,11 +118,11 @@ const useLoadedTeamRaw = ( const data = emptyLoadedTeamData(validTeamID) return listMeta ? {...data, teamMeta: listMeta} : data }, [validTeamID, listMeta]) - const {data, loaded, loading, reload, clear} = useCachedResource({ - cache, + const {data, loaded, loading, reload} = useCachedResource({ cacheKey: validTeamID, - enabled: enabled && !!validTeamID, + enabled, initialData, + invalidateOn: teamInvalidations(validTeamID), load: async () => { const teamIDToLoad = validTeamID ?? T.Teams.noTeamID const [annotatedTeam] = await Promise.all([ @@ -134,6 +134,7 @@ const useLoadedTeamRaw = ( teamMeta: annotatedTeamToMeta(teamIDToLoad, annotatedTeam, undefined), } }, + namespace: loadedTeams, onError: error => { logger.warn(`Failed to load team data for ${validTeamID}`, error) }, @@ -156,45 +157,6 @@ const useLoadedTeamRaw = ( ) const yourOperations = React.useMemo(() => Teams.deriveCanPerform(roleAndDetails), [roleAndDetails]) - // One logical change fires metadata, role map and changedByID, and a reconnect - // fires all three at once - measured as 4 getAnnotatedTeam for one team inside - // 116ms, each a separate event superseding the last. Coalesce them the way - // useReloadOnTeamChanges does for the teams list: leading so the common single - // notification still reloads immediately, trailing to catch the rest of a burst. - const reloadNow = React.useEffectEvent(() => { - if (enabled) { - void reload() - } - }) - const [debouncedReload] = React.useState void>>(() => - debounce(() => reloadNow(), 2000, {leading: true, trailing: true}) - ) - React.useEffect(() => { - return () => { - debouncedReload.cancel() - } - }, [debouncedReload]) - const onTeamChange = () => { - debouncedReload() - } - useEngineActionListener('keybase.1.NotifyTeam.teamMetadataUpdate', onTeamChange, subscribeToUpdates) - useEngineActionListener('keybase.1.NotifyTeam.teamRoleMapChanged', onTeamChange, subscribeToUpdates) - useEngineActionListener('keybase.1.NotifyTeam.teamChangedByID', action => { - if (action.payload.params.teamID === validTeamID) { - onTeamChange() - } - }, subscribeToUpdates) - useEngineActionListener('keybase.1.NotifyTeam.teamDeleted', action => { - if (enabled && action.payload.params.teamID === validTeamID) { - clear(validTeamID) - } - }, subscribeToUpdates) - useEngineActionListener('keybase.1.NotifyTeam.teamExit', action => { - if (enabled && action.payload.params.teamID === validTeamID) { - clear(validTeamID) - } - }, subscribeToUpdates) - const teamDetails = data.teamDetails return React.useMemo( () => ({loaded, loading, reload, teamDetails, teamMeta, yourOperations}), @@ -212,11 +174,6 @@ export const LoadedTeamProvider = (props: React.PropsWithChildren<{teamID: T.Tea export const useLoadedTeam = (teamID: T.Teams.TeamID, enabled = true): LoadedTeam => { const context = React.useContext(LoadedTeamContext) const useContextValue = context?.teamID === teamID - const raw = useLoadedTeamRaw( - teamID, - enabled && !useContextValue, - enabled && !useContextValue, - useContextValue - ) + const raw = useLoadedTeamRaw(teamID, enabled && !useContextValue) return useContextValue ? context : raw } diff --git a/shared/teams/use-teams-list.tsx b/shared/teams/use-teams-list.tsx index ee25ff4f86e0..beeaee13b2c7 100644 --- a/shared/teams/use-teams-list.tsx +++ b/shared/teams/use-teams-list.tsx @@ -1,22 +1,17 @@ import * as C from '@/constants' -import type {DebouncedFunc} from 'lodash' -import debounce from 'lodash/debounce' import isEqual from 'lodash/isEqual' import logger from '@/logger' import {useConfigState} from '@/stores/config' import {useCurrentUserState} from '@/stores/current-user' import * as Teams from '@/constants/teams' import {ensureError} from '@/util/errors' -import {nextReloadEpoch} from '@/util/reload-epoch' -import {useEngineActionListener} from '@/engine/action-listener' import * as React from 'react' import * as T from '@/constants/types' import { - type CachedResourceCache, - createCachedResourceCache, + type CachedResourceInvalidation, + createCachedResourceNamespace, useCachedResource, } from '@/util/use-cached-resource' -import {registerExternalResetter} from '@/util/zustand' type TeamsList = { reload: () => void @@ -35,23 +30,46 @@ const TeamsListContext = React.createContext(null) const TeamsRoleMapContext = React.createContext(null) const teamsListReloadStaleMs = 5 * 60_000 -const teamsListInvalidationListeners = new Set<(epoch: number) => void>() -const teamsRoleMapInvalidationListeners = new Set<(epoch: number) => void>() -const teamsListCache = createCachedResourceCache, string | undefined>( - emptyTeams, - undefined +// Both are keyed by username, and every read goes through peek(currentUsername), +// so the previous user's list can never be rendered. Their entries are dropped by +// the namespace's sign-out reset, which is what keeps a re-login inside the stale +// window from being served them. +const teamsListResource = createCachedResourceNamespace, string>( + 'teams-list-cache', + () => emptyTeams ) -const teamsRoleMapCache = createCachedResourceCache( - emptyTeamRoleMap, - undefined +const teamsRoleMapResource = createCachedResourceNamespace( + 'teams-role-map-cache', + () => emptyTeamRoleMap ) -// module scope outlives sign-out; both are keyed by username, so the next user -// would briefly render the previous user's team list and role map -registerExternalResetter('teams-list-caches', () => { - teamsListCache.reset(emptyTeams, undefined) - teamsRoleMapCache.reset(emptyTeamRoleMap, undefined) -}) +// Reads for consumers rendered outside the provider. Keyed on the current user +// so an entry the previous one left behind can never be rendered. +const peekTeams = () => { + const username = useCurrentUserState.getState().username + return username ? teamsListResource.peek(username) : emptyTeams +} +const peekRoleMap = () => { + const username = useCurrentUserState.getState().username + return username ? teamsRoleMapResource.peek(username) : emptyTeamRoleMap +} + +// reload whenever the service signals a team change. One logical change fires +// several of these; useCachedResource coalesces the burst onto one reload. +const makeTeamChangeInvalidations = (includeMetadataUpdate: boolean) => + [ + ...(includeMetadataUpdate + ? ([{type: 'keybase.1.NotifyTeam.teamMetadataUpdate'}] as const) + : ([] as const)), + {type: 'keybase.1.NotifyTeam.teamRoleMapChanged'}, + {type: 'keybase.1.NotifyTeam.teamChangedByID'}, + {type: 'keybase.1.NotifyTeam.teamDeleted'}, + {type: 'keybase.1.NotifyTeam.teamExit'}, + ] satisfies ReadonlyArray + +// Incoming team chat messages fire teamMetadataUpdate; only the list cares. +const teamsListInvalidations = makeTeamChangeInvalidations(true) +const teamsRoleMapInvalidations = makeTeamChangeInvalidations(false) const teamListToArray = (list: ReadonlyArray) => { return [...Teams.teamListToMeta(list).values()] @@ -62,7 +80,7 @@ const teamListToArray = (list: ReadonlyArray) => { // array when nothing changed) so context consumers like TeamsRoot can bail. const recycleTeamList = ( old: ReadonlyArray, - next: Array + next: ReadonlyArray ): ReadonlyArray => { if (old.length === next.length && next.every((t, i) => isEqual(t, old[i]))) { return old @@ -74,75 +92,14 @@ const recycleTeamList = ( }) } -const invalidateCachedResource = (cache: CachedResourceCache, nextKey: K) => { - cache.invalidate(nextKey) -} - export const invalidateLoadedTeams = () => { const username = useCurrentUserState.getState().username const loggedIn = useConfigState.getState().loggedIn if (!loggedIn || !username) { return } - invalidateCachedResource(teamsListCache, username) - invalidateCachedResource(teamsRoleMapCache, username) - // one invalidation is one event: every listener reloads against the same - // epoch so they share an rpc instead of superseding each other - const epoch = nextReloadEpoch() - teamsListInvalidationListeners.forEach(listener => listener(epoch)) - teamsRoleMapInvalidationListeners.forEach(listener => listener(epoch)) -} - -// reload whenever the service signals a team change or invalidateLoadedTeams fires -const useReloadOnTeamChanges = ( - enabled: boolean, - reload: (epoch?: number) => unknown, - invalidationListeners: Set<(epoch: number) => void>, - includeMetadataUpdate = false -) => { - const reloadNow = React.useEffectEvent(() => { - if (enabled) { - void reload() - } - }) - // service notifications arrive in bursts (one logical change can fire metadata, - // role map, and changedByID); coalesce so a burst costs at most a leading and - // a trailing reload instead of one per event. Lazy ref init is the sanctioned - // create-once exception: a restarted render just recreates the debouncer. - const debouncedReloadRef = React.useRef void> | null>(null) - if (debouncedReloadRef.current == null) { - debouncedReloadRef.current = debounce(() => reloadNow(), 2000, {leading: true, trailing: true}) - } - React.useEffect(() => { - return () => { - debouncedReloadRef.current?.cancel() - } - }, []) - const onChange = () => { - debouncedReloadRef.current?.() - } - useEngineActionListener('keybase.1.NotifyTeam.teamMetadataUpdate', () => { - if (includeMetadataUpdate) { - onChange() - } - }) - useEngineActionListener('keybase.1.NotifyTeam.teamRoleMapChanged', onChange) - useEngineActionListener('keybase.1.NotifyTeam.teamChangedByID', onChange) - useEngineActionListener('keybase.1.NotifyTeam.teamDeleted', onChange) - useEngineActionListener('keybase.1.NotifyTeam.teamExit', onChange) - - React.useEffect(() => { - if (!enabled) { - return - } - const listener = (epoch: number) => { - void reload(epoch) - } - invalidationListeners.add(listener) - return () => { - invalidationListeners.delete(listener) - } - }, [enabled, reload, invalidationListeners]) + teamsListResource.invalidate(username) + teamsRoleMapResource.invalidate(username) } const useTeamsListRaw = (enabled = true): TeamsList => { @@ -150,28 +107,28 @@ const useTeamsListRaw = (enabled = true): TeamsList => { const loggedIn = useConfigState(s => s.loggedIn) const loadTeamsRPC = C.useRPC(T.RPCGen.teamsTeamListUnverifiedRpcPromise) const {data: teams, reload} = useCachedResource({ - cache: teamsListCache, - cacheKey: username, - enabled: enabled && !!username && loggedIn, + cacheKey: username || undefined, + enabled: enabled && loggedIn, initialData: emptyTeams, + invalidateOn: teamsListInvalidations, load: async () => new Promise>((resolve, reject) => { loadTeamsRPC( [{includeImplicitTeams: false, userAssertion: username}, C.waitingKeyTeamsLoaded], - result => resolve(recycleTeamList(teamsListCache.getData(), teamListToArray(result.teams ?? []))), + result => resolve(teamListToArray(result.teams ?? [])), error => reject(ensureError(error)) ) }), + namespace: teamsListResource, onError: error => { if ((error as {code?: number}).code !== T.RPCGen.StatusCode.scapinetworkerror) { logger.warn('Failed to load teams list', error) } }, + recycle: recycleTeamList, staleMs: teamsListReloadStaleMs, }) - useReloadOnTeamChanges(enabled, reload, teamsListInvalidationListeners, true) - return React.useMemo(() => ({reload, teams}), [reload, teams]) } @@ -184,10 +141,10 @@ const useTeamsRoleMapRaw = (enabled = true): TeamsRoleMap => { loadIfStale, reload, } = useCachedResource({ - cache: teamsRoleMapCache, - cacheKey: username, - enabled: enabled && !!username && loggedIn, + cacheKey: username || undefined, + enabled: enabled && loggedIn, initialData: emptyTeamRoleMap, + invalidateOn: teamsRoleMapInvalidations, load: async () => new Promise((resolve, reject) => { loadRoleMapRPC( @@ -196,6 +153,7 @@ const useTeamsRoleMapRaw = (enabled = true): TeamsRoleMap => { error => reject(ensureError(error)) ) }), + namespace: teamsRoleMapResource, onError: error => { if ((error as {code?: number}).code !== T.RPCGen.StatusCode.scapinetworkerror) { logger.warn('Failed to load teams role map', error) @@ -204,8 +162,6 @@ const useTeamsRoleMapRaw = (enabled = true): TeamsRoleMap => { staleMs: teamsListReloadStaleMs, }) - useReloadOnTeamChanges(enabled, reload, teamsRoleMapInvalidationListeners) - return React.useMemo(() => ({loadIfStale, reload, roleMap}), [loadIfStale, reload, roleMap]) } @@ -229,7 +185,7 @@ export const useTeamsList = (): TeamsList => { const context = React.useContext(TeamsListContext) // read the cache every render (not a one-time snapshot) so provider-less // consumers still see fresh data; identity stays stable while data does - const teams = teamsListCache.getData() + const teams = peekTeams() const fallback = React.useMemo(() => ({reload: noopLoad, teams}), [teams]) return context ?? fallback } @@ -240,14 +196,14 @@ export const useTeamsList = (): TeamsList => { // throwing. The cache stays fresh because the provider is mounted elsewhere. export const useTeamsRoleMap = (): TeamsRoleMap => { const context = React.useContext(TeamsRoleMapContext) - const roleMap = teamsRoleMapCache.getData() + const roleMap = peekRoleMap() const fallback = React.useMemo(() => ({loadIfStale: noopLoad, reload: noopLoad, roleMap}), [roleMap]) return context ?? fallback } export const useTeamsListMap = () => { const context = React.useContext(TeamsListContext) - const teams = context?.teams ?? teamsListCache.getData() + const teams = context?.teams ?? peekTeams() return React.useMemo(() => new Map(teams.map(team => [team.id, team] as const)), [teams]) } @@ -256,6 +212,6 @@ export const useTeamsListNameToIDMap = () => { // (popup-root is a sibling to the router, outside LoadedTeamsListProvider), so // fall back to the module cache instead of throwing when there's no provider. const context = React.useContext(TeamsListContext) - const teams = context?.teams ?? teamsListCache.getData() + const teams = context?.teams ?? peekTeams() return React.useMemo(() => new Map(teams.map(team => [team.teamname, team.id] as const)), [teams]) } diff --git a/shared/util/use-cached-resource.test.tsx b/shared/util/use-cached-resource.test.tsx index e750ffa1fb90..fb84ea1a94e7 100644 --- a/shared/util/use-cached-resource.test.tsx +++ b/shared/util/use-cached-resource.test.tsx @@ -4,8 +4,10 @@ import {afterEach, expect, jest, test} from '@jest/globals' import {act, cleanup, render, renderHook} from '@testing-library/react' import {useDaemonState} from '@/stores/daemon' import {nextReloadEpoch} from './reload-epoch' -import {createCachedResourceCache, useCachedResource} from './use-cached-resource' +import {createCachedResourceNamespace, useCachedResource} from './use-cached-resource' import {flush} from '@/test/flush' +import {notifyEngineActionListeners} from '@/engine/action-listener' +import {resetAllStores} from '@/util/zustand' afterEach(() => { cleanup() @@ -14,10 +16,16 @@ afterEach(() => { type Data = {v: number} +// a fresh namespace per test: they are module-scope in real callers, and reusing +// one here would leak a loaded entry into the next test +let namespaceCount = 0 +const makeNamespace = (initialData: T) => + createCachedResourceNamespace(`test-${namespaceCount++}`, () => initialData) + // A caller that rebuilds initialData every render (seeding it from another // store) must not put useCachedResource into a render loop. test('unstable initialData does not loop', async () => { - const cache = createCachedResourceCache({v: 0}, 'k') + const namespace = makeNamespace({v: 0}) let calls = 0 let renders = 0 const load = jest.fn(async () => { @@ -29,7 +37,7 @@ test('unstable initialData does not loop', async () => { // counts real renders: compiling this away is exactly what the test measures 'use no memo' renders++ - const {data} = useCachedResource({cache, cacheKey: 'k', initialData: {v: 0}, load, staleMs: 5000}) + const {data} = useCachedResource({namespace, cacheKey: 'k', initialData: {v: 0}, load, staleMs: 5000}) return
{data.v}
} render() @@ -42,7 +50,7 @@ test('unstable initialData does not loop', async () => { // backoff every re-render re-issued the request the instant the previous one // settled, which hammered both the service and the server. test('a failed load backs off instead of retrying on every render', async () => { - const cache = createCachedResourceCache({v: 0}, 'k') + const namespace = makeNamespace({v: 0}) let calls = 0 let loadIfStale: (() => Promise) | undefined const load = jest.fn(async () => { @@ -55,7 +63,7 @@ test('a failed load backs off instead of retrying on every render', async () => // so without this the assertion below holds even with no backoff at all 'use no memo' const resource = useCachedResource({ - cache, + namespace, cacheKey: 'k', initialData: {v: 0}, load, @@ -92,7 +100,7 @@ test('a failed load backs off instead of retrying on every render', async () => }) test('reload bypasses the failure backoff', async () => { - const cache = createCachedResourceCache({v: 0}, 'k') + const namespace = makeNamespace({v: 0}) let calls = 0 let reload: (() => Promise) | undefined const load = jest.fn(async () => { @@ -104,7 +112,7 @@ test('reload bypasses the failure backoff', async () => { // hoists reload out to the test body; the compiler rejects the assignment 'use no memo' const resource = useCachedResource({ - cache, + namespace, cacheKey: 'k', initialData: {v: 0}, load, @@ -124,7 +132,7 @@ test('reload bypasses the failure backoff', async () => { }) test('a successful load is served from cache while fresh', async () => { - const cache = createCachedResourceCache({v: 0}, 'k') + const namespace = makeNamespace({v: 0}) let calls = 0 const load = jest.fn(async () => { calls++ @@ -132,7 +140,7 @@ test('a successful load is served from cache while fresh', async () => { return {v: calls} }) const Comp = ({staleMs}: {staleMs: number}) => { - const {data, loaded} = useCachedResource({cache, cacheKey: 'k', initialData: {v: 0}, load, staleMs}) + const {data, loaded} = useCachedResource({namespace, cacheKey: 'k', initialData: {v: 0}, load, staleMs}) return
{loaded ? data.v : 'x'}
} const first = render() @@ -145,7 +153,7 @@ test('a successful load is served from cache while fresh', async () => { const second = render() await flush() expect(calls).toBe(1) - expect(cache.getData()).toEqual({v: 1}) + expect(namespace.peek('k')).toEqual({v: 1}) // and a consumer that considers it stale does reload it second.unmount() @@ -158,7 +166,7 @@ test('a successful load is served from cache while fresh', async () => { // already on the wire before the change: joining it would serve pre-change data // AND stamp loadedAt on it, pinning the stale value for the whole window. test('a forced reload supersedes a request that predates it', async () => { - const cache = createCachedResourceCache({v: 0}, 'k') + const namespace = makeNamespace({v: 0}) let calls = 0 const releases: Array<(v: Data) => void> = [] const load = jest.fn(async () => { @@ -170,7 +178,7 @@ test('a forced reload supersedes a request that predates it', async () => { let reload: (() => Promise) | undefined const Comp = () => { 'use no memo' - const resource = useCachedResource({cache, cacheKey: 'k', initialData: {v: 0}, load, staleMs: 5000}) + const resource = useCachedResource({namespace, cacheKey: 'k', initialData: {v: 0}, load, staleMs: 5000}) reload = resource.reload return
{resource.loaded ? `v${resource.data.v}` : 'pending'}
} @@ -197,14 +205,14 @@ test('a forced reload supersedes a request that predates it', async () => { }) await flush() expect(view.getAllByText('v2')).toHaveLength(1) - expect(cache.getData()).toEqual({v: 2}) + expect(namespace.peek('k')).toEqual({v: 2}) }) // The mutation and the reload it triggers routinely fall inside one millisecond, // so ordering the two by Date.now() compares them equal and the forced load // joins the very request it exists to supersede. test('a forced reload supersedes a same-millisecond request', async () => { - const cache = createCachedResourceCache({v: 0}, 'k') + const namespace = makeNamespace({v: 0}) let calls = 0 const releases: Array<(v: Data) => void> = [] const load = jest.fn(async () => { @@ -216,7 +224,7 @@ test('a forced reload supersedes a same-millisecond request', async () => { let reload: (() => Promise) | undefined const Comp = () => { 'use no memo' - const resource = useCachedResource({cache, cacheKey: 'k', initialData: {v: 0}, load, staleMs: 5000}) + const resource = useCachedResource({namespace, cacheKey: 'k', initialData: {v: 0}, load, staleMs: 5000}) reload = resource.reload return
{resource.loaded ? `v${resource.data.v}` : 'pending'}
} @@ -241,7 +249,7 @@ test('a forced reload supersedes a same-millisecond request', async () => { // the property the module-level caches depend on: without it, sharing a cache // across screens still issues an RPC per screen. test('concurrent consumers of one cache share a single load', async () => { - const cache = createCachedResourceCache({v: 0}, 'k') + const namespace = makeNamespace({v: 0}) let calls = 0 let release: ((v: Data) => void) | undefined const load = jest.fn(async () => { @@ -251,7 +259,7 @@ test('concurrent consumers of one cache share a single load', async () => { }) }) const Comp = () => { - const {data, loaded} = useCachedResource({cache, cacheKey: 'k', initialData: {v: 0}, load, staleMs: 5000}) + const {data, loaded} = useCachedResource({namespace, cacheKey: 'k', initialData: {v: 0}, load, staleMs: 5000}) return
{loaded ? `v${data.v}` : 'pending'}
} const view = render( @@ -278,7 +286,7 @@ test('concurrent consumers of one cache share a single load', async () => { // the first supersede its predecessor - N rpcs for one event. Measured as 4 // identical getAnnotatedTeam inside 106ms after one reconnect. test('consumers reloading for one event share a single rpc', async () => { - const cache = createCachedResourceCache({v: 0}, 'k') + const namespace = makeNamespace({v: 0}) let calls = 0 const releases: Array<(v: Data) => void> = [] const load = jest.fn(async () => { @@ -290,7 +298,7 @@ test('consumers reloading for one event share a single rpc', async () => { const reloads: Array<(epoch?: number) => Promise> = [] const Comp = () => { 'use no memo' - const resource = useCachedResource({cache, cacheKey: 'k', initialData: {v: 0}, load, staleMs: 5000}) + const resource = useCachedResource({namespace, cacheKey: 'k', initialData: {v: 0}, load, staleMs: 5000}) reloads.push(resource.reload) return
{resource.loaded ? `v${resource.data.v}` : 'pending'}
} @@ -328,7 +336,7 @@ test('consumers reloading for one event share a single rpc', async () => { // teams list reloaded 75ms apart for one reconnect, and the first request had // already settled, so the in-flight check had nothing to collapse onto. test('a consumer reloading for an event already in the cache does not refetch', async () => { - const cache = createCachedResourceCache({v: 0}, 'k') + const namespace = makeNamespace({v: 0}) let calls = 0 const releases: Array<(v: Data) => void> = [] const load = jest.fn(async () => { @@ -340,7 +348,7 @@ test('a consumer reloading for an event already in the cache does not refetch', const reloads: Array<(epoch?: number) => Promise> = [] const Comp = () => { 'use no memo' - const resource = useCachedResource({cache, cacheKey: 'k', initialData: {v: 0}, load, staleMs: 5000}) + const resource = useCachedResource({namespace, cacheKey: 'k', initialData: {v: 0}, load, staleMs: 5000}) reloads.push(resource.reload) return
{resource.loaded ? `v${resource.data.v}` : 'pending'}
} @@ -389,7 +397,7 @@ test('a consumer reloading for an event already in the cache does not refetch', // The collapse must not swallow a genuinely newer event: a reload for a later // epoch still supersedes whatever an earlier one put on the wire. test('a later epoch still supersedes an in-flight request', async () => { - const cache = createCachedResourceCache({v: 0}, 'k') + const namespace = makeNamespace({v: 0}) let calls = 0 const releases: Array<(v: Data) => void> = [] const load = jest.fn(async () => { @@ -401,7 +409,7 @@ test('a later epoch still supersedes an in-flight request', async () => { let reload: ((epoch?: number) => Promise) | undefined const Comp = () => { 'use no memo' - const resource = useCachedResource({cache, cacheKey: 'k', initialData: {v: 0}, load, staleMs: 5000}) + const resource = useCachedResource({namespace, cacheKey: 'k', initialData: {v: 0}, load, staleMs: 5000}) reload = resource.reload return
{resource.loaded ? `v${resource.data.v}` : 'pending'}
} @@ -432,7 +440,7 @@ test('a later epoch still supersedes an in-flight request', async () => { }) await flush() expect(view.getAllByText('v3')).toHaveLength(1) - expect(cache.getData()).toEqual({v: 3}) + expect(namespace.peek('k')).toEqual({v: 3}) }) // End to end over the wiring that actually produced the burst: one reconnect, @@ -441,7 +449,7 @@ test('a reconnect reloads every consumer with one rpc', async () => { act(() => { useDaemonState.setState({handshakeGeneration: 1, handshakeState: 'done'}) }) - const cache = createCachedResourceCache({v: 0}, 'k') + const namespace = makeNamespace({v: 0}) let calls = 0 const releases: Array<(v: Data) => void> = [] const load = jest.fn(async () => { @@ -451,7 +459,7 @@ test('a reconnect reloads every consumer with one rpc', async () => { }) }) const Comp = () => { - const {data, loaded} = useCachedResource({cache, cacheKey: 'k', initialData: {v: 0}, load, staleMs: 5000}) + const {data, loaded} = useCachedResource({namespace, cacheKey: 'k', initialData: {v: 0}, load, staleMs: 5000}) return
{loaded ? `v${data.v}` : 'pending'}
} const view = render( @@ -487,7 +495,7 @@ test('a reconnect reloads every consumer with one rpc', async () => { // An engine reset orphans in-flight rpcs without ever settling them. A forced // load must not adopt one of those, or reload() never resolves. test('reload() bypasses an orphaned in-flight request', async () => { - const cache = createCachedResourceCache('', 'k') + const namespace = makeNamespace('') let resolveSecond: ((v: string) => void) | undefined const load = jest .fn<() => Promise>() @@ -499,7 +507,7 @@ test('reload() bypasses an orphaned in-flight request', async () => { }) ) const {result} = renderHook(() => - useCachedResource({cache, cacheKey: 'k', initialData: '', load, staleMs: 10_000}) + useCachedResource({namespace, cacheKey: 'k', initialData: '', load, staleMs: 10_000}) ) await flush() expect(load).toHaveBeenCalledTimes(1) @@ -524,14 +532,14 @@ test('a disabled resource ignores reconnects until it is enabled', async () => { act(() => { useDaemonState.setState({handshakeGeneration: 1, handshakeState: 'done'}) }) - const cache = createCachedResourceCache({v: 0}, 'k') + const namespace = makeNamespace({v: 0}) const load = jest.fn(async () => { await Promise.resolve() return {v: 1} }) const {rerender, result} = renderHook( ({enabled}: {enabled: boolean}) => - useCachedResource({cache, cacheKey: 'k', enabled, initialData: {v: 0}, load, staleMs: 5000}), + useCachedResource({namespace, cacheKey: 'k', enabled, initialData: {v: 0}, load, staleMs: 5000}), {initialProps: {enabled: false}} ) await flush() @@ -553,32 +561,56 @@ test('a disabled resource ignores reconnects until it is enabled', async () => { expect(result.current.data).toEqual({v: 1}) }) -// Nothing runs loadResource while a hook is disabled, so becoming disabled is -// the only chance to drop data the cache is still holding under the old key. -test('a resource that is disabled while its key changes clears the stale key', async () => { - const cache = createCachedResourceCache({v: 0}, 'a') +// The hazard every consumer used to hand-roll around: a disabled instance - a +// shadow behind a provider, an off-screen row, an id that has not resolved - +// reset whatever cache object it was handed, wiping a real loader's data and +// costing a refetch on the next mount. With no key of its own it has no shared +// entry to reset. +test('a disabled instance never touches the shared entry', async () => { + const namespace = makeNamespace({v: 0}) + let calls = 0 const load = jest.fn(async () => { + calls++ await Promise.resolve() - return {v: 1} + return {v: calls} }) - const {rerender} = renderHook( - ({cacheKey, enabled}: {cacheKey: string; enabled: boolean}) => - useCachedResource({cache, cacheKey, enabled, initialData: {v: 0}, load, staleMs: 5000}), - {initialProps: {cacheKey: 'a', enabled: true}} - ) + const props = {cacheKey: 'k', initialData: {v: 0}, load, namespace, staleMs: 5000} + + const loader = renderHook(() => useCachedResource({...props, enabled: true})) await flush() - expect(cache.getKey()).toBe('a') - expect(cache.getLoadedAt()).not.toBe(0) + expect(calls).toBe(1) + loader.unmount() - rerender({cacheKey: 'b', enabled: false}) + const disabled = renderHook(() => useCachedResource({...props, enabled: false})) await flush() - expect(cache.getKey()).toBe('b') - expect(cache.getLoadedAt()).toBe(0) - expect(load).toHaveBeenCalledTimes(1) + disabled.unmount() + + // still inside the stale window, so the entry must still be there + expect(namespace.peek('k')).toEqual({v: 1}) + const again = renderHook(() => useCachedResource({...props, enabled: true})) + await flush() + expect(calls).toBe(1) + expect(again.result.current.data).toEqual({v: 1}) +}) + +// An instance with no key has nothing shared to load into either, and must not +// seed an entry a later real loader would find already present but empty. +test('an instance with no key loads nothing and seeds no entry', async () => { + const namespace = makeNamespace({v: 0}) + const load = jest.fn(async () => { + await Promise.resolve() + return {v: 1} + }) + renderHook(() => + useCachedResource({cacheKey: undefined, initialData: {v: 0}, load, namespace, staleMs: 5000}) + ) + await flush() + expect(load).not.toHaveBeenCalled() + expect(namespace.peek('k')).toEqual({v: 0}) }) test('clear() drops the cached data and reloads', async () => { - const cache = createCachedResourceCache({v: 0}, 'k') + const namespace = makeNamespace({v: 0}) let calls = 0 const load = jest.fn(async () => { calls++ @@ -586,7 +618,7 @@ test('clear() drops the cached data and reloads', async () => { return {v: calls} }) const {result} = renderHook(() => - useCachedResource({cache, cacheKey: 'k', initialData: {v: 0}, load, staleMs: 5000}) + useCachedResource({namespace, cacheKey: 'k', initialData: {v: 0}, load, staleMs: 5000}) ) await flush() expect(calls).toBe(1) @@ -598,7 +630,7 @@ test('clear() drops the cached data and reloads', async () => { }) expect(result.current.data).toEqual({v: 0}) expect(result.current.loaded).toBe(false) - expect(cache.getLoadedAt()).toBe(0) + expect(namespace.peek('k')).toEqual({v: 0}) // clear() invalidates rather than just blanking state: the next stale check // has to go back to the wire even though staleMs has not elapsed. @@ -610,13 +642,13 @@ test('clear() drops the cached data and reloads', async () => { expect(result.current.data).toEqual({v: 2}) }) -test('a cacheKey change resets the cache and refetches', async () => { - const cache = createCachedResourceCache({v: 0}, 'a') +test('a cacheKey change loads the new key, and each key keeps its own entry', async () => { + const namespace = makeNamespace({v: 0}) const seen: Array = [] const {rerender, result} = renderHook( ({cacheKey}: {cacheKey: string}) => useCachedResource({ - cache, + namespace, cacheKey, initialData: {v: 0}, load: async () => { @@ -635,13 +667,130 @@ test('a cacheKey change resets the cache and refetches', async () => { rerender({cacheKey: 'b'}) await flush() expect(seen).toEqual(['a', 'b']) - expect(cache.getKey()).toBe('b') expect(result.current.data).toEqual({v: 2}) - // data for the old key must not resurface when the key comes back: the reset - // dropped it, so this is a fresh load rather than a cache hit. + // one entry per key, so coming back inside the stale window is a hit rather + // than a refetch - and 'a' can never be served under key 'b' rerender({cacheKey: 'a'}) await flush() - expect(seen).toEqual(['a', 'b', 'a']) - expect(result.current.data).toEqual({v: 3}) + expect(seen).toEqual(['a', 'b']) + expect(result.current.data).toEqual({v: 1}) + expect(namespace.peek('b')).toEqual({v: 2}) +}) + +// The entries are per-user data and module scope outlives a sign-out. +test('a sign-out reset empties every entry in the namespace', async () => { + const namespace = makeNamespace({v: 0}) + let calls = 0 + const load = jest.fn(async () => { + calls++ + await Promise.resolve() + return {v: calls} + }) + const props = {cacheKey: 'k', initialData: {v: 0}, load, namespace, staleMs: 5000} + const first = renderHook(() => useCachedResource(props)) + await flush() + expect(calls).toBe(1) + first.unmount() + + act(() => { + resetAllStores() + }) + expect(namespace.peek('k')).toEqual({v: 0}) + + renderHook(() => useCachedResource(props)) + await flush() + expect(calls).toBe(2) +}) + +const homeRefresh = {payload: {params: {}}, type: 'keybase.1.homeUI.homeUIRefresh'} as never + +// Service notifications are coalesced over a 2s window, but an explicit +// invalidate() is already one event with its own epoch, and it has zeroed +// loadedAt - deferring it to the trailing edge leaves every consumer rendering +// empty for the rest of the window instead of for one round trip. +test('an explicit invalidate reloads now even inside an open coalescing window', async () => { + const namespace = makeNamespace({v: 0}) + let calls = 0 + const load = async () => { + calls++ + await Promise.resolve() + return {v: calls} + } + renderHook(() => + useCachedResource({ + cacheKey: 'k', + initialData: {v: 0}, + invalidateOn: [{type: 'keybase.1.homeUI.homeUIRefresh'}], + load, + namespace, + staleMs: 5000, + }) + ) + await flush() + expect(calls).toBe(1) + + // leading edge of the coalescing window + act(() => { + notifyEngineActionListeners(homeRefresh) + }) + await flush() + expect(calls).toBe(2) + + act(() => { + namespace.invalidate('k') + }) + await flush() + expect(calls).toBe(3) +}) + +// A queued trailing reload would re-issue the rpc for the very team that was +// just deleted or left, and repopulate the entry the clear dropped. +test('a clear cancels a reload queued by an earlier notification', async () => { + jest.useFakeTimers({doNotFake: ['nextTick', 'setImmediate']}) + const namespace = makeNamespace({v: 0}) + let calls = 0 + const load = async () => { + calls++ + await Promise.resolve() + return {v: calls} + } + renderHook(() => + useCachedResource({ + cacheKey: 'k', + initialData: {v: 0}, + invalidateOn: [ + {type: 'keybase.1.homeUI.homeUIRefresh'}, + {effect: 'clear', type: 'keybase.1.NotifyBadges.badgeState'}, + ], + load, + namespace, + staleMs: 5000, + }) + ) + await flush() + expect(calls).toBe(1) + + // two inside one window: leading fires now, trailing is queued + act(() => { + notifyEngineActionListeners(homeRefresh) + notifyEngineActionListeners(homeRefresh) + }) + await flush() + expect(calls).toBe(2) + + act(() => { + notifyEngineActionListeners({payload: {params: {}}, type: 'keybase.1.NotifyBadges.badgeState'} as never) + }) + await flush() + expect(namespace.peek('k')).toEqual({v: 0}) + + act(() => { + jest.advanceTimersByTime(5000) + }) + await flush() + // the trailing edge must not have fired: clear() is deliberate, and a reload + // behind it would put the dropped entry straight back + expect(calls).toBe(2) + jest.useRealTimers() }) diff --git a/shared/util/use-cached-resource.tsx b/shared/util/use-cached-resource.tsx index 8b14e27863bd..14028fdd6aa7 100644 --- a/shared/util/use-cached-resource.tsx +++ b/shared/util/use-cached-resource.tsx @@ -1,7 +1,12 @@ import * as C from '@/constants' import * as React from 'react' +import debounce from 'lodash/debounce' +import type {DebouncedFunc} from 'lodash' +import type * as EngineGen from '@/constants/rpc' import {produce} from 'immer' import {joinAnyEpoch, nextReloadEpoch} from './reload-epoch' +import {subscribeToEngineAction} from '@/engine/action-listener' +import {registerExternalResetter} from '@/util/zustand' import {useReloadOnReconnect} from './use-reload-on-reconnect' export type CachedResourceCache = { @@ -40,17 +45,52 @@ type StoredCachedResourceState = CachedResourceState & { initialData: T } +/** + * One engine action that should reload (or drop) this resource. `when` narrows + * it to the entry this instance holds - the same notification fires for every + * team in the app. + */ +export type CachedResourceInvalidation = { + type: EngineGen.ActionType + when?: (action: EngineGen.Actions) => boolean + effect?: 'reload' | 'clear' +} + type Props = { - cache: CachedResourceCache - cacheKey: K + /** + * Where the entry lives. Omit for a resource nothing else shares: it then gets + * a cache of its own, private to this instance. + */ + namespace?: CachedResourceNamespace + /** + * Which entry in the namespace. `undefined` means this instance has no entry: + * it shares nothing and resets nothing, so an instance that is off (a shadow + * behind a provider, an unresolved id) cannot clobber a loader's data. It is + * also what stops the resource loading, together with `enabled`. + */ + cacheKey?: K enabled?: boolean initialData: T load: () => Promise onError?: (error: unknown) => void + /** + * Reuse identities from the previously cached value. A load that produces a + * deep-equal result should hand back the object already in the cache so + * downstream memos can bail. + */ + recycle?: (previous: T, next: T) => T + /** Engine actions (and the namespace's own invalidations) that reload this. */ + invalidateOn?: ReadonlyArray refreshKey?: unknown staleMs: number } +// One logical change fires several notifications - metadata, role map and +// changedByID all land for a single team edit, and a reconnect fires all of them +// at once. Coalesce: leading so the common single notification still reloads +// immediately, trailing to catch the rest of a burst. +const invalidationDebounceMs = 2000 + const emptyState = (data: T): CachedResourceState => ({ data, loaded: false, @@ -155,7 +195,7 @@ export const createCachedResourceCache = (initialData: T, key: K): CachedR } } -export const getCachedResourceCache = ( +const getCachedResourceCache = ( map: Map>, initialData: T, key: K @@ -169,11 +209,102 @@ export const getCachedResourceCache = ( return created } +/** + * A named family of cache entries - the map, the sign-out reset and the + * invalidation broadcast that every consumer used to hand-roll. Create one per + * module, at module scope. + */ +export type CachedResourceNamespace = { + /** current value for a key without creating an entry, for provider-less reads */ + peek: (key: K) => T + /** drop the entry and tell every mounted consumer of it to reload, as one event */ + invalidate: (key: K) => void + /** same, for every entry at once */ + invalidateAll: () => void + /** internal: the entry a mounted consumer loads into */ + getCache: (key: K) => CachedResourceCache + /** internal: consumers listen for invalidate() */ + subscribe: (listener: (key: K, epoch: number) => void) => () => void +} + +export const createCachedResourceNamespace = ( + id: string, + initialDataForKey: (key: K) => T, + // Cap entries when one is expensive to hold. Least recently asked for goes + // first: clearing the whole map at the cap makes the key you switch back to + // reload even though its entry was still fresh. + options?: {maxEntries?: number} +): CachedResourceNamespace => { + const {maxEntries} = options ?? {} + const caches = new Map>() + const listeners = new Set<(key: K, epoch: number) => void>() + + // module scope outlives sign-out, and these entries are per-user data. Dropping + // the map is not enough on its own: a consumer still mounted through the + // sign-out holds the cache object itself, so each one is emptied as well. + registerExternalResetter(id, () => { + caches.forEach((cache, key) => { + cache.reset(initialDataForKey(key), key) + }) + caches.clear() + }) + + const notify = (key: K, epoch: number) => { + for (const listener of [...listeners]) { + listener(key, epoch) + } + } + + return { + getCache: key => { + if (maxEntries !== undefined) { + // Map iterates in insertion order, so re-inserting on use is what makes + // that order recency. + const existing = caches.get(key) + if (existing) { + caches.delete(key) + caches.set(key, existing) + } else { + while (caches.size >= maxEntries) { + const oldest = caches.keys().next() + if (oldest.done) { + break + } + caches.delete(oldest.value) + } + } + } + return getCachedResourceCache(caches, initialDataForKey(key), key) + }, + invalidate: key => { + caches.get(key)?.invalidate(key) + // one invalidation is one event: every listener reloads against the same + // epoch so they share an rpc instead of superseding each other + notify(key, nextReloadEpoch()) + }, + invalidateAll: () => { + const epoch = nextReloadEpoch() + for (const key of [...caches.keys()]) { + caches.get(key)?.invalidate(key) + notify(key, epoch) + } + }, + peek: key => caches.get(key)?.getData() ?? initialDataForKey(key), + subscribe: listener => { + listeners.add(listener) + return () => { + listeners.delete(listener) + } + }, + } +} + const runLoad = async ( cache: CachedResourceCache, cacheKey: K, initialData: T, load: () => Promise, + recycle: ((previous: T, next: T) => T) | undefined, onError: ((error: unknown) => void) | undefined, requestVersion: number, requestVersionRef: React.RefObject, @@ -201,7 +332,8 @@ const runLoad = async ( } return } - request = load().then(data => { + request = load().then(raw => { + const data = recycle ? recycle(cache.getData(), raw) : raw cache.setDataLoaded(data, generation, epoch) return data }) @@ -231,7 +363,38 @@ const runLoad = async ( } export const useCachedResource = (props: Props) => { - const {cache, cacheKey, enabled = true, initialData, load, onError, refreshKey, staleMs} = props + const { + cacheKey: requestedKey, + enabled: requestedEnabled = true, + initialData, + invalidateOn, + load, + namespace, + onError, + recycle, + refreshKey, + staleMs, + } = props + // An instance with no key of its own shares nothing, so it also cannot reset + // anything shared - which is what a disabled or unresolved instance would + // otherwise do to the loader's entry. + const shared = !!namespace && requestedKey !== undefined && requestedEnabled + const [privateCache] = React.useState(() => + createCachedResourceCache(initialData, undefined) + ) + const cache = React.useMemo( + () => + shared + ? (namespace.getCache(requestedKey) as CachedResourceCache) + : privateCache, + [namespace, privateCache, requestedKey, shared] + ) as CachedResourceCache + // Without a namespace the private cache is this instance's own, so it keeps the + // real key and a key change still resets and refetches through the usual path. + const cacheKey = (shared || !namespace ? requestedKey : undefined) as K + // The key requirement is what makes an entry-less instance inert; without a + // namespace there is no entry to be inert about. + const enabled = requestedEnabled && (namespace === undefined || requestedKey !== undefined) const [state, setState] = React.useState>(() => storedState(cache, cacheKey, initialData, cachedState(cache, cacheKey, initialData)) ) @@ -243,8 +406,10 @@ export const useCachedResource = (props: Props) => { cacheKey, enabled, initialData, + invalidateOn, load, onError, + recycle, staleMs, }) React.useLayoutEffect(() => { @@ -253,11 +418,13 @@ export const useCachedResource = (props: Props) => { cacheKey, enabled, initialData, + invalidateOn, load, onError, + recycle, staleMs, } - }, [cache, cacheKey, enabled, initialData, load, onError, staleMs]) + }) // deliberately does not depend on initialData: resetCache is in the main // effect's dep array, and callers routinely rebuild initialData (seeding it @@ -279,7 +446,7 @@ export const useCachedResource = (props: Props) => { ) const loadResource = React.useCallback(async (force: boolean, epoch: number) => { - const {cache, cacheKey, enabled, initialData, load, onError, staleMs} = latestRef.current + const {cache, cacheKey, enabled, initialData, load, onError, recycle, staleMs} = latestRef.current const resetCache = (nextKey: K) => { cache.reset(initialData, nextKey) } @@ -323,6 +490,7 @@ export const useCachedResource = (props: Props) => { cacheKey, initialData, load, + recycle, onError, requestVersion, requestVersionRef, @@ -347,6 +515,83 @@ export const useCachedResource = (props: Props) => { await loadResource(false, joinAnyEpoch) }, [loadResource]) + // One trigger for every invalidation source - engine notifications and the + // namespace's own invalidate(). The debounce comes BEFORE the epoch: a burst + // that coalesces into one reload is one event, and an epoch allocated per + // notification would make each reload supersede the last. An invalidate() + // broadcast carries its epoch through so every consumer of it shares one rpc. + const invalidateNow = React.useEffectEvent((epoch?: number) => { + void loadResource(true, typeof epoch === 'number' ? epoch : nextReloadEpoch()) + }) + // useEffectEvent hands back a new wrapper identity every render (only its inner + // ref is stable), so the debouncer is built once around the first wrapper; it + // stays valid because every wrapper shares that ref. + const [debouncedInvalidate] = React.useState void>>(() => + debounce((epoch?: number) => invalidateNow(epoch), invalidationDebounceMs, { + leading: true, + trailing: true, + }) + ) + // Cancel on the way out AND whenever the entry changes: a trailing reload + // queued for the old key would otherwise fire against the new one, allocate a + // fresh epoch and supersede the load already in flight for it. + React.useEffect( + () => () => { + debouncedInvalidate.cancel() + }, + [cache, cacheKey, debouncedInvalidate] + ) + const clearNow = React.useEffectEvent(() => { + // A queued trailing reload would re-issue the rpc for the very team that was + // just deleted or left, and repopulate the entry this is dropping. + debouncedInvalidate.cancel() + clear(latestRef.current.cacheKey) + }) + + // joined rather than passed as an array so the effect below has a stable dep: + // callers build invalidateOn inline, and its predicates close over this render + // deduped: two entries for one type (a reload and a clear, say) must not mean + // two subscriptions, each of which would then run the whole array + const invalidateTypes = [...new Set((invalidateOn ?? []).map(entry => entry.type))].join('|') + React.useEffect(() => { + if (!enabled || !invalidateTypes) { + return + } + const unsubs = invalidateTypes.split('|').map(type => + subscribeToEngineAction(type as EngineGen.ActionType, action => { + for (const entry of latestRef.current.invalidateOn ?? []) { + if (entry.type !== action.type || (entry.when && !entry.when(action))) { + continue + } + if (entry.effect === 'clear') { + clearNow() + } else { + debouncedInvalidate() + } + } + }) + ) + return () => { + for (const unsub of unsubs) unsub() + } + }, [debouncedInvalidate, enabled, invalidateTypes]) + + // a mounted consumer picks up an invalidate() too, not just the next mount + React.useEffect(() => { + if (!shared) { + return + } + return namespace.subscribe((invalidatedKey, epoch) => { + if (Object.is(invalidatedKey, requestedKey)) { + // Straight through, not through the debounce: an invalidate() is already + // one event carrying one epoch, and it has zeroed loadedAt, so deferring + // it to a trailing edge leaves every consumer rendering empty for the + // rest of the window instead of for one round trip. + invalidateNow(epoch) + } + }) + }, [namespace, requestedKey, shared]) + // reconnects orphan any in-flight load; force so cached data from before the // restart doesn't mask post-restart changes. Disabled hooks must not touch the // shared cache (loadResource resets it when disabled) diff --git a/shared/util/use-mutual-teams.tsx b/shared/util/use-mutual-teams.tsx index 6d4a02bfdad3..3e4c249cb858 100644 --- a/shared/util/use-mutual-teams.tsx +++ b/shared/util/use-mutual-teams.tsx @@ -1,12 +1,7 @@ import * as React from 'react' import * as T from '@/constants/types' import logger from '@/logger' -import { - type CachedResourceCache, - getCachedResourceCache, - useCachedResource, -} from '@/util/use-cached-resource' -import {registerExternalResetter} from '@/util/zustand' +import {createCachedResourceNamespace, useCachedResource} from '@/util/use-cached-resource' // getMutualTeamsLocal makes the service localize every conversation the users // share, and each of those remotely refreshes its participant list - one call @@ -18,18 +13,11 @@ const mutualTeamsStaleMs = 60_000 const emptyTeams: ReadonlyArray = [] -type MutualTeamsCacheMap = Map< - string, - CachedResourceCache, string> -> - -const mutualTeamsCache: MutualTeamsCacheMap = new Map() - -// module scope outlives sign-out and "teams you share with X" is per-user -registerExternalResetter('mutual-teams-cache', () => { - mutualTeamsCache.forEach((cache, key) => cache.reset(emptyTeams, key)) - mutualTeamsCache.clear() -}) +// "teams you share with X" is per-user +const mutualTeamsResource = createCachedResourceNamespace, string>( + 'mutual-teams-cache', + () => emptyTeams +) // order-independent: two callers listing the same people must hit the same entry const mutualTeamsKey = (usernames: ReadonlyArray) => [...usernames].sort().join(',') @@ -45,19 +33,9 @@ export const useMutualTeams = ( const cacheKey = mutualTeamsKey(usernames) // deliberately not gated on a non-empty username list: the service treats the // empty case as a real query, and skipping it would change what callers get - const canLoad = enabled - // a disabled instance resets whatever cache it holds, so it must never hold - // the shared one - const [localCacheMap] = React.useState(() => new Map()) - const cacheMap = canLoad ? mutualTeamsCache : localCacheMap - const cache = React.useMemo( - () => getCachedResourceCache(cacheMap, emptyTeams, cacheKey), - [cacheMap, cacheKey] - ) const {data, loaded, loading} = useCachedResource({ - cache, cacheKey, - enabled: canLoad, + enabled, initialData: emptyTeams, load: async () => { const res = await T.RPCChat.localGetMutualTeamsLocalRpcPromise( @@ -66,6 +44,7 @@ export const useMutualTeams = ( ) return res.teams ?? emptyTeams }, + namespace: mutualTeamsResource, onError: error => { logger.warn(`Failed to load mutual teams for ${cacheKey}`, error) },