From b2fed16f58cb8814c6c3298bdd167bdc9628db3f Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Wed, 9 Sep 2026 14:57:26 -0400 Subject: [PATCH] refactor(cache): move the cache lifecycle inside useCachedResource useCachedResource resets whatever cache object it is handed while disabled. That is an ordering constraint on the caller stated in no type, so eight consumers each answered "which cache object do I hold this render?" with the same ~40-line scaffold: a module-scope map, a registerExternalResetter, an invalidation listener Set, a useXCacheMap gate, a debounce, several useEngineActionListeners, and a recycler inside load(). The hook now takes a namespace and a key instead of a cache object. The namespace owns the map and the sign-out reset. `key: undefined` means the instance has no entry: it shares nothing and resets nothing, so the shadow hazard is unrepresentable rather than something each caller has to gate against - chat/conversation/team-hooks gated on forceLocalCache alone, not on !enabled || !validTeamID like its sibling, and could seed and reset a shared entry an instance had no business holding. invalidateOn folds the engine listeners, the invalidation Sets and the debounce into one trigger. The debounce runs BEFORE the epoch is allocated, so a burst that coalesces into one reload is one event rather than several that supersede each other; an invalidate() broadcast carries its own epoch through so every consumer of it shares a single rpc. recycle moves the identity recyclers out of load(), where they had to reach into the cache object to see the previous value. Team channels and chat team members now coalesce their reload bursts the way the team and the teams list already did. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015rccpV5nLxxC5opF5xzrz7 --- shared/chat/conversation/team-hooks.tsx | 106 +++---- shared/chat/user-emoji.tsx | 57 +--- shared/teams/common/activity.tsx | 27 +- shared/teams/common/general-conv.tsx | 40 +-- .../teams/common/use-loaded-team-channels.tsx | 188 +++++-------- .../team/settings-tab/default-channels.tsx | 24 +- .../team/settings-tab/retention/index.tsx | 58 ++-- shared/teams/team/use-loaded-team.test.tsx | 52 ++++ shared/teams/team/use-loaded-team.tsx | 139 ++++----- shared/teams/use-teams-list.tsx | 156 ++++------- shared/util/use-cached-resource.test.tsx | 263 ++++++++++++++---- shared/util/use-cached-resource.tsx | 259 ++++++++++++++++- shared/util/use-mutual-teams.tsx | 37 +-- 13 files changed, 787 insertions(+), 619 deletions(-) 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) },