From 932c1640e83223bc6dd4935116c0e5548557bbe1 Mon Sep 17 00:00:00 2001 From: sumo_slonik Date: Thu, 9 Jul 2026 12:43:16 +0200 Subject: [PATCH 1/4] Speed up SearchRouter open on large accounts Cache createFilteredOptionList results between opens, defer building contact options until the user types a query, and clear the caches on sign-out. --- .../Search/SearchAutocompleteList.tsx | 8 +- src/hooks/useFilteredOptions.ts | 36 +++-- src/hooks/usePrivateIsArchivedMap.ts | 43 +++++- src/hooks/useSearchSelector/base.ts | 1 - src/libs/OptionsListUtils/index.ts | 125 +++++++++++++++++- src/libs/SessionCleanup.ts | 24 ++++ src/libs/actions/Session/index.ts | 2 + src/pages/NewChatPage/index.tsx | 1 - src/pages/Share/ShareTab.tsx | 1 - tests/perf-test/OptionsListUtils.perf-test.ts | 12 +- tests/unit/OptionsListUtilsTest.tsx | 21 +++ 11 files changed, 244 insertions(+), 30 deletions(-) create mode 100644 src/libs/SessionCleanup.ts diff --git a/src/components/Search/SearchAutocompleteList.tsx b/src/components/Search/SearchAutocompleteList.tsx index 87a95a3d50be..5e0490f3acae 100644 --- a/src/components/Search/SearchAutocompleteList.tsx +++ b/src/components/Search/SearchAutocompleteList.tsx @@ -200,7 +200,13 @@ function SearchAutocompleteList({ const taxRates = useMemo(() => getAllTaxRates(policies), [policies]); const [isTrackIntentUser] = useOnyx(ONYXKEYS.NVP_INTRO_SELECTED, {selector: isTrackIntentUserSelector}); - const {options: listOptions, isLoading: isLoadingOptions} = useFilteredOptions({enabled: true, isSearching: !!autocompleteQueryValue.trim(), betas: betas ?? []}); + const {options: listOptions, isLoading: isLoadingOptions} = useFilteredOptions({ + enabled: true, + isSearching: !!autocompleteQueryValue.trim(), + // The empty-query state renders only recent searches and recent reports (no standalone contacts), + // so contacts can be deferred until the user types a query. + deferContactsUntilSearch: true, + }); const isRecentSearchesDataLoaded = !isLoadingOnyxValue(recentSearchesMetadata); diff --git a/src/hooks/useFilteredOptions.ts b/src/hooks/useFilteredOptions.ts index 53cf0487baa9..005b8edab812 100644 --- a/src/hooks/useFilteredOptions.ts +++ b/src/hooks/useFilteredOptions.ts @@ -2,13 +2,11 @@ import {createFilteredOptionList} from '@libs/OptionsListUtils'; import type {OptionList} from '@libs/OptionsListUtils/types'; import ONYXKEYS from '@src/ONYXKEYS'; -import type Beta from '@src/types/onyx/Beta'; - -import type {OnyxEntry} from 'react-native-onyx'; import {isTrackIntentUserSelector} from '@selectors/Onboarding'; import {useMemo, useState} from 'react'; +import useLocalize from './useLocalize'; import useOnyx from './useOnyx'; import usePrivateIsArchivedMap from './usePrivateIsArchivedMap'; import useReportAttributes from './useReportAttributes'; @@ -26,8 +24,12 @@ type UseFilteredOptionsConfig = { enablePagination?: boolean; /** Whether search mode is active - when true, builds full report map for personal details (default: false) */ isSearching?: boolean; - /** Beta features the user has access to */ - betas?: OnyxEntry; + /** + * When true, contacts (personal details) are only built while searching. Use on screens whose + * idle/empty state does not show standalone contacts (e.g. the SearchRouter) to avoid building + * an option per contact on open. Leave false for contact pickers (default: false). + */ + deferContactsUntilSearch?: boolean; }; type UseFilteredOptionsResult = { @@ -62,7 +64,6 @@ type UseFilteredOptionsResult = { * const {options, isLoading} = useFilteredOptions({ * maxRecentReports: 500, * enabled: didScreenTransitionEnd, - * betas, * }); * * */ function useFilteredOptions(config: UseFilteredOptionsConfig = {}): UseFilteredOptionsResult { - const {maxRecentReports = 500, enabled = true, includeP2P = true, batchSize = 100, isSearching = false, betas} = config; + const {maxRecentReports = 500, enabled = true, includeP2P = true, batchSize = 100, isSearching = false, deferContactsUntilSearch = false} = config; const [reportsLimit, setReportsLimit] = useState(maxRecentReports); @@ -81,6 +82,9 @@ function useFilteredOptions(config: UseFilteredOptionsConfig = {}): UseFilteredO const reportAttributesDerived = useReportAttributes(); const [isTrackIntentUser] = useOnyx(ONYXKEYS.NVP_INTRO_SELECTED, {selector: isTrackIntentUserSelector}); + // Option building is locale-dependent, so a consumer that stays mounted through a language switch recomputes. + const {preferredLocale} = useLocalize(); + const privateIsArchivedMap = usePrivateIsArchivedMap(); const totalReports = allReports ? Object.keys(allReports).length : 0; @@ -99,14 +103,28 @@ function useFilteredOptions(config: UseFilteredOptionsConfig = {}): UseFilteredO maxRecentReports: reportsLimit, includeP2P, isSearching, - betas, + deferContactsUntilSearch, + locale: preferredLocale, }, undefined, undefined, isTrackIntentUser, ) : null, - [enabled, allReports, allPersonalDetails, reportAttributesDerived, privateIsArchivedMap, allPolicies, reportsLimit, includeP2P, isSearching, betas, isTrackIntentUser], + [ + enabled, + allReports, + allPersonalDetails, + reportAttributesDerived, + privateIsArchivedMap, + allPolicies, + reportsLimit, + includeP2P, + isSearching, + deferContactsUntilSearch, + preferredLocale, + isTrackIntentUser, + ], ); // When isSearching is set to true, the createFilteredOptionList returns all reports diff --git a/src/hooks/usePrivateIsArchivedMap.ts b/src/hooks/usePrivateIsArchivedMap.ts index 393e2cc4174b..a538ce91518c 100644 --- a/src/hooks/usePrivateIsArchivedMap.ts +++ b/src/hooks/usePrivateIsArchivedMap.ts @@ -1,14 +1,35 @@ +import {registerSessionCleanupCallback} from '@libs/SessionCleanup'; + import ONYXKEYS from '@src/ONYXKEYS'; +import type {ReportNameValuePairs} from '@src/types/onyx'; + +import type {OnyxCollection} from 'react-native-onyx'; import useOnyx from './useOnyx'; type PrivateIsArchivedMap = Record; -/** - * Hook that returns a map of report IDs to their private_isArchived values - */ -function usePrivateIsArchivedMap(): PrivateIsArchivedMap { - const [allReportNVP] = useOnyx(ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS); +// Single-entry cache so the derived map keeps a stable reference across renders and remounts +// while the underlying REPORT_NAME_VALUE_PAIRS collection is referentially unchanged. +// Reference equality on the source is reliable: Onyx hands every subscriber the same frozen +// collection snapshot and rebuilds it only when a member changes (see OnyxCache.getCollectionData +// in react-native-onyx), and a changed reference only costs a map rebuild here. +// The initial `undefined` doubles as the "nothing cached yet" marker: it only matches an equally +// undefined (unloaded) collection, for which the correct result is the empty map anyway. +let cachedSource: OnyxCollection; +let cachedMap: PrivateIsArchivedMap = {}; + +// The cached collection belongs to the signed-in account, so release it on sign-out +// rather than holding it until the next subscriber mounts. +registerSessionCleanupCallback(() => { + cachedSource = undefined; + cachedMap = {}; +}); + +function buildPrivateIsArchivedMap(allReportNVP: OnyxCollection): PrivateIsArchivedMap { + if (allReportNVP === cachedSource) { + return cachedMap; + } const map: PrivateIsArchivedMap = {}; if (allReportNVP) { @@ -16,8 +37,20 @@ function usePrivateIsArchivedMap(): PrivateIsArchivedMap { map[key] = !!value?.private_isArchived; } } + + cachedSource = allReportNVP; + cachedMap = map; return map; } +/** + * Hook that returns a map of report IDs to their private_isArchived values. + * The returned map is a shared cached reference — treat it as read-only; mutating it corrupts the cache. + */ +function usePrivateIsArchivedMap(): PrivateIsArchivedMap { + const [allReportNVP] = useOnyx(ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS); + return buildPrivateIsArchivedMap(allReportNVP); +} + export default usePrivateIsArchivedMap; export type {PrivateIsArchivedMap}; diff --git a/src/hooks/useSearchSelector/base.ts b/src/hooks/useSearchSelector/base.ts index 519702c1bb0d..bcb1003aca08 100644 --- a/src/hooks/useSearchSelector/base.ts +++ b/src/hooks/useSearchSelector/base.ts @@ -218,7 +218,6 @@ function useSearchSelectorBase({ enabled: shouldInitialize, isSearching: isSearchingOptions, batchSize: maxResultsPerPage, - betas, }); const areOptionsInitialized = !isLoadingOptions; const defaultOptions = filteredOptions ?? emptyOptionList; diff --git a/src/libs/OptionsListUtils/index.ts b/src/libs/OptionsListUtils/index.ts index d819c026c5d6..3632de5628a7 100644 --- a/src/libs/OptionsListUtils/index.ts +++ b/src/libs/OptionsListUtils/index.ts @@ -163,15 +163,18 @@ import { shouldReportBeInOptionList, shouldShowMarkAsDone, } from '@libs/ReportUtils'; +import {registerSessionCleanupCallback} from '@libs/SessionCleanup'; import StringUtils from '@libs/StringUtils'; import {getTaskCreatedMessage, getTaskReportActionMessage} from '@libs/TaskUtils'; import {getDescription, getAmount as getTransactionAmount, getCurrency as getTransactionCurrency, isScanning} from '@libs/TransactionUtils'; import {generateAccountID} from '@libs/UserUtils'; import CONST from '@src/CONST'; +import IntlStore from '@src/languages/IntlStore'; import ONYXKEYS from '@src/ONYXKEYS'; import type { Beta, + Locale, Login, OnyxInputOrEntry, PersonalDetails, @@ -247,10 +250,17 @@ const deprecatedAllSortedReportActions: Record = {}; const deprecatedCachedOneTransactionThreadReportIDs: Record = {}; /** @deprecated Use sortedReportActionsData from ONYXKEYS.DERIVED.RAM_ONLY_SORTED_REPORT_ACTIONS instead. Will be removed once all flows are migrated. */ let deprecatedAllReportActions: OnyxCollection; + +// The sorted report-actions objects above are mutated in place (their references never change), +// so this version number is the only signal that their contents were updated. +let deprecatedReportActionsVersion = 0; + Onyx.connect({ key: ONYXKEYS.COLLECTION.REPORT_ACTIONS, waitForCollectionCallback: true, callback: (actions) => { + // Bump before the empty guard so clearing the collection also invalidates caches keyed on this version. + deprecatedReportActionsVersion++; if (!actions) { return; } @@ -1577,6 +1587,47 @@ const reportSortComparator = (report: Report, privateIsArchivedMap: PrivateIsArc * * Use this for screens that need recent reports (NewChatPage, WorkspaceInvitePage, etc.) */ +// Shared stable default so an omitted `visibleReportActionsData` keeps a constant reference across calls, +// which the cache below relies on for hits. +const EMPTY_VISIBLE_REPORT_ACTIONS: VisibleReportActionsDerivedValue = {}; + +// Cache keyed by the options signature so each selection screen configuration (SearchRouter, NewChatPage, +// ShareTab, etc.) keeps its own entry and reopening one screen is not evicted by opening another. +// An entry is reused only while its Onyx inputs are referentially unchanged. +const filteredOptionListCache = new Map(); + +// One slot per active screen configuration (~7 distinct callers) plus a small buffer. +// The LRU bound prevents a paginating screen from flooding the cache with one entry +// per distinct maxRecentReports value and evicting entries for other screens. +const FILTERED_OPTION_LIST_CACHE_MAX_ENTRIES = 8; + +/** Builds the cache key from the option values that define a distinct screen configuration. */ +function buildFilteredOptionListCacheKey(args: Array): string { + return args.join('_'); +} + +// Consumers (e.g. getValidOptions) mutate option objects in place (isBold/isSelected/brickRoadIndicator), +// so the cache keeps a pristine copy and every caller receives its own shallow clones, matching the +// per-call fresh objects they would get without the cache. +// NOTE: this is a shallow clone — the top-level fields consumers mutate today are all scalars. Nested +// objects (icons, participantsList, item, allReportErrors) stay shared with the cached entry, so any new +// consumer that mutates those in place would corrupt the cache and must clone them first. +function cloneOptionList(optionList: OptionList): OptionList { + return { + reports: optionList.reports.map((option) => ({...option})), + personalDetails: optionList.personalDetails.map((option) => ({...option})), + }; +} + +/** Clears the createFilteredOptionList cache. For tests that measure or exercise the build path with unchanged inputs. */ +function clearFilteredOptionListCache() { + filteredOptionListCache.clear(); +} + +// The cached results (and the Onyx collection references in their keys) belong to the signed-in +// account, so drop them on sign-out instead of holding them until the next call. +registerSessionCleanupCallback(() => filteredOptionListCache.clear()); + function createFilteredOptionList( personalDetails: OnyxEntry, reports: OnyxCollection, @@ -1587,13 +1638,55 @@ function createFilteredOptionList( maxRecentReports?: number; includeP2P?: boolean; isSearching?: boolean; - betas?: OnyxEntry; + /** + * When true, personal details (contacts) are only built while searching (`isSearching`). + * For screens whose idle/empty state shows no standalone contacts (e.g. the SearchRouter), + * this skips building an option for every contact on open. Screens that show contacts at + * empty state (contact pickers) must leave this false. + */ + deferContactsUntilSearch?: boolean; + locale?: Locale; } = {}, policyTags?: OnyxCollection, - visibleReportActionsData: VisibleReportActionsDerivedValue = {}, + visibleReportActionsData: VisibleReportActionsDerivedValue = EMPTY_VISIBLE_REPORT_ACTIONS, isTrackIntentUser?: boolean, -) { - const {maxRecentReports = 500, includeP2P = true, isSearching = false} = options; +): OptionList { + const {maxRecentReports = 500, includeP2P = true, isSearching = false, deferContactsUntilSearch = false, locale} = options; + + // Contacts are expensive to build on large accounts (one option per personal detail). When a screen + // opts into deferral and is not actively searching, skip building them entirely; the empty state + // does not display standalone contacts, and typing flips `isSearching` which rebuilds the full set. + const areContactsDeferred = deferContactsUntilSearch && !isSearching; + const shouldBuildContacts = includeP2P && !areContactsDeferred; + + // Search-mode results contain an option for every report and contact, so caching them would retain + // full-account-sized arrays until sign-out — and any Onyx change invalidates them anyway. + const shouldUseCache = !isSearching; + + const cacheEntryKey = buildFilteredOptionListCacheKey([maxRecentReports, includeP2P, isSearching, deferContactsUntilSearch]); + const cacheInputs = [ + personalDetails, + reports, + reportAttributesDerived, + privateIsArchivedMap, + policiesCollection, + policyTags, + visibleReportActionsData, + isTrackIntentUser, + // Option building translates strings imperatively (translateLocal), so the active locale is part of the output. + locale ?? IntlStore.getCurrentLocale(), + // Option building reads module-level sorted report actions (getLastMessageTextForReport) that are + // mutated in place, so their version stands in for the never-changing object reference. + deprecatedReportActionsVersion, + ]; + const cachedEntry = shouldUseCache ? filteredOptionListCache.get(cacheEntryKey) : undefined; + if (cachedEntry && cacheInputs.every((value, index) => value === cachedEntry.inputs.at(index))) { + // Re-inserting refreshes recency so a frequently-hit entry is not evicted by writes to other keys. + filteredOptionListCache.delete(cacheEntryKey); + filteredOptionListCache.set(cacheEntryKey, cachedEntry); + return cloneOptionList(cachedEntry.result); + } + const reportMapForAccountIDs: Record = {}; // Step 1: Pre-filter reports to avoid processing thousands @@ -1669,8 +1762,8 @@ function createFilteredOptionList( } } - // Step 5: Process personal details (all of them - needed for search functionality) - const personalDetailsOptions = includeP2P + // Step 5: Process personal details (all of them when built - needed for search functionality) + const personalDetailsOptions = shouldBuildContacts ? Object.values(personalDetails ?? {}).map((personalDetail) => { const accountID = personalDetail?.accountID ?? CONST.DEFAULT_NUMBER_ID; @@ -1695,10 +1788,27 @@ function createFilteredOptionList( }) : []; - return { + const result: OptionList = { reports: reportOptions, personalDetails: personalDetailsOptions as Array>, }; + + if (!shouldUseCache) { + return result; + } + + // Re-inserting moves the entry to the end of the Map, so eviction below drops the least recently used entry. + filteredOptionListCache.delete(cacheEntryKey); + if (filteredOptionListCache.size >= FILTERED_OPTION_LIST_CACHE_MAX_ENTRIES) { + const oldestEntryKey = filteredOptionListCache.keys().next().value; + if (oldestEntryKey !== undefined) { + filteredOptionListCache.delete(oldestEntryKey); + } + } + filteredOptionListCache.set(cacheEntryKey, {inputs: cacheInputs, result}); + + // The caller gets clones because the cached entry must stay pristine (see cloneOptionList). + return cloneOptionList(result); } function createOptionFromReport( @@ -3358,6 +3468,7 @@ function processSearchString(searchString: string | undefined): string[] { export { canCreateOptimisticPersonalDetailOption, + clearFilteredOptionListCache, combineOrderingOfReportsAndPersonalDetails, createOptionFromReport, createFilteredOptionList, diff --git a/src/libs/SessionCleanup.ts b/src/libs/SessionCleanup.ts new file mode 100644 index 000000000000..ff606c7c7186 --- /dev/null +++ b/src/libs/SessionCleanup.ts @@ -0,0 +1,24 @@ +type SessionCleanupCallback = () => void; + +const callbacks: SessionCleanupCallback[] = []; + +/** + * Registers a callback that releases module-level state holding the signed-in account's data + * (e.g. memory caches keyed by Onyx collections). Registered callbacks run on sign-out via + * `runSessionCleanupCallbacks` (called from `cleanupSession` in the Session actions). + * + * This module is dependency-free on purpose: Session actions can import it without pulling in + * (and creating import cycles with) the modules that own the caches. + */ +function registerSessionCleanupCallback(callback: SessionCleanupCallback) { + callbacks.push(callback); +} + +/** Runs all registered cleanup callbacks. Called on sign-out. */ +function runSessionCleanupCallbacks() { + for (const callback of callbacks) { + callback(); + } +} + +export {registerSessionCleanupCallback, runSessionCleanupCallbacks}; diff --git a/src/libs/actions/Session/index.ts b/src/libs/actions/Session/index.ts index eba84e1f68c0..14240ea709b1 100644 --- a/src/libs/actions/Session/index.ts +++ b/src/libs/actions/Session/index.ts @@ -36,6 +36,7 @@ import * as SequentialQueue from '@libs/Network/SequentialQueue'; import Pusher from '@libs/Pusher'; import reauthenticate from '@libs/Reauthentication'; import {getReportIDFromLink} from '@libs/ReportUtils'; +import {runSessionCleanupCallbacks} from '@libs/SessionCleanup'; import * as SessionUtils from '@libs/SessionUtils'; import {checkIfShouldUseNewPartnerName, resetDidUserLogInDuringSession} from '@libs/SessionUtils'; import {clearSoundAssetsCache} from '@libs/Sound'; @@ -1074,6 +1075,7 @@ function cleanupSession() { }); clearCachedAttachments(); clearSoundAssetsCache(); + runSessionCleanupCallbacks(); } function clearAccountMessages() { diff --git a/src/pages/NewChatPage/index.tsx b/src/pages/NewChatPage/index.tsx index 0fb63ea5a43c..55d022838018 100755 --- a/src/pages/NewChatPage/index.tsx +++ b/src/pages/NewChatPage/index.tsx @@ -93,7 +93,6 @@ function useOptions(reportAttributesDerived: ReportAttributesDerivedValue['repor batchSize: 100, enablePagination: true, isSearching, - betas, }); const [allPolicies] = useOnyx(ONYXKEYS.COLLECTION.POLICY); diff --git a/src/pages/Share/ShareTab.tsx b/src/pages/Share/ShareTab.tsx index 53c6e6f0c12d..8b8429f98cab 100644 --- a/src/pages/Share/ShareTab.tsx +++ b/src/pages/Share/ShareTab.tsx @@ -60,7 +60,6 @@ function ShareTab() { const {didScreenTransitionEnd} = useScreenWrapperTransitionStatus(); const {options: listOptions, isLoading} = useFilteredOptions({ enabled: didScreenTransitionEnd, - betas: betas ?? [], isSearching: !!debouncedTextInputValue.trim(), }); const areOptionsInitialized = !isLoading; diff --git a/tests/perf-test/OptionsListUtils.perf-test.ts b/tests/perf-test/OptionsListUtils.perf-test.ts index 47cd39caa4e5..f4f5c121e0ce 100644 --- a/tests/perf-test/OptionsListUtils.perf-test.ts +++ b/tests/perf-test/OptionsListUtils.perf-test.ts @@ -1,6 +1,6 @@ import type {PrivateIsArchivedMap} from '@hooks/usePrivateIsArchivedMap'; -import {createFilteredOptionList, filterAndOrderOptions, getSearchOptions, getValidOptions} from '@libs/OptionsListUtils'; +import {clearFilteredOptionListCache, createFilteredOptionList, filterAndOrderOptions, getSearchOptions, getValidOptions} from '@libs/OptionsListUtils'; import type {OptionData} from '@libs/ReportUtils'; import CONST from '@src/CONST'; @@ -276,12 +276,14 @@ describe('OptionsListUtils', () => { test('[OptionsListUtils] createFilteredOptionList', async () => { await waitForBatchedUpdates(); - await measureFunction(() => - createFilteredOptionList(personalDetails, mockedReportsMap, undefined, EMPTY_PRIVATE_IS_ARCHIVED_MAP, undefined, { + await measureFunction(() => { + // Inputs are referentially identical across measured runs, so clear the cache to measure the build path. + clearFilteredOptionListCache(); + return createFilteredOptionList(personalDetails, mockedReportsMap, undefined, EMPTY_PRIVATE_IS_ARCHIVED_MAP, undefined, { maxRecentReports: 500, isSearching: false, - }), - ); + }); + }); }); test('[OptionsListUtils] createFilteredOptionList with isSearching is true', async () => { diff --git a/tests/unit/OptionsListUtilsTest.tsx b/tests/unit/OptionsListUtilsTest.tsx index 78bf6df78fc4..845dd72e7bb1 100644 --- a/tests/unit/OptionsListUtilsTest.tsx +++ b/tests/unit/OptionsListUtilsTest.tsx @@ -14,6 +14,7 @@ import isSearchTopmostFullScreenRoute from '@libs/Navigation/helpers/isSearchTop import type {OptionList, Options, SearchOption, SearchOptionData} from '@libs/OptionsListUtils'; import { canCreateOptimisticPersonalDetailOption, + clearFilteredOptionListCache, createFilteredOptionList, createOption, createOptionFromReport, @@ -857,6 +858,11 @@ describe('OptionsListUtils', () => { ); }); + // createFilteredOptionList caches results at module level; clear it so tests stay order-independent. + beforeEach(() => { + clearFilteredOptionListCache(); + }); + describe('getSearchOptions()', () => { it('should return all options when no search value is provided', () => { // Given a set of options @@ -8328,6 +8334,21 @@ describe('OptionsListUtils', () => { expect(result).toHaveProperty('reports'); expect(result).toHaveProperty('personalDetails'); }); + + // The SearchRouter relies on this: its empty-query state renders recent reports only, + // so contacts must not be built until the user starts searching. + it('should not build personal details when deferContactsUntilSearch is true and not searching', () => { + const result = createFilteredOptionList(PERSONAL_DETAILS, REPORTS, undefined, {}, undefined, {deferContactsUntilSearch: true}); + + expect(result.reports.length).toBeGreaterThan(0); + expect(result.personalDetails.length).toBe(0); + }); + + it('should build personal details when deferContactsUntilSearch is true and searching', () => { + const result = createFilteredOptionList(PERSONAL_DETAILS, REPORTS, undefined, {}, undefined, {deferContactsUntilSearch: true, isSearching: true}); + + expect(result.personalDetails.length).toBeGreaterThan(0); + }); }); describe('getFilteredRecentAttendees', () => { it('should deduplicate recent attendees by email', () => { From 52cce1f14c7f2a40a07a2afd7dcd142ff2171d06 Mon Sep 17 00:00:00 2001 From: sumo_slonik Date: Thu, 9 Jul 2026 12:43:16 +0200 Subject: [PATCH 2/4] Add dev-only mutation guards to shared caches --- src/hooks/usePrivateIsArchivedMap.ts | 12 ++++--- src/libs/OptionsListUtils/index.ts | 31 ++++++++++++++++++ src/libs/SessionCleanup.ts | 13 ++++++-- tests/unit/OptionsListUtilsTest.tsx | 40 +++++++++++++++++++++++ tests/unit/SessionCleanupTest.ts | 17 ++++++++++ tests/unit/usePrivateIsArchivedMapTest.ts | 10 ++++++ 6 files changed, 115 insertions(+), 8 deletions(-) create mode 100644 tests/unit/SessionCleanupTest.ts diff --git a/src/hooks/usePrivateIsArchivedMap.ts b/src/hooks/usePrivateIsArchivedMap.ts index a538ce91518c..db50e931b9b8 100644 --- a/src/hooks/usePrivateIsArchivedMap.ts +++ b/src/hooks/usePrivateIsArchivedMap.ts @@ -7,7 +7,8 @@ import type {OnyxCollection} from 'react-native-onyx'; import useOnyx from './useOnyx'; -type PrivateIsArchivedMap = Record; +// Readonly because consumers receive a shared cached reference (see below) — a write would corrupt it for every other consumer. +type PrivateIsArchivedMap = Readonly>; // Single-entry cache so the derived map keeps a stable reference across renders and remounts // while the underlying REPORT_NAME_VALUE_PAIRS collection is referentially unchanged. @@ -31,7 +32,7 @@ function buildPrivateIsArchivedMap(allReportNVP: OnyxCollection = {}; if (allReportNVP) { for (const [key, value] of Object.entries(allReportNVP)) { map[key] = !!value?.private_isArchived; @@ -39,13 +40,14 @@ function buildPrivateIsArchivedMap(allReportNVP: OnyxCollection void; const callbacks: SessionCleanupCallback[] = []; @@ -7,8 +9,8 @@ const callbacks: SessionCleanupCallback[] = []; * (e.g. memory caches keyed by Onyx collections). Registered callbacks run on sign-out via * `runSessionCleanupCallbacks` (called from `cleanupSession` in the Session actions). * - * This module is dependency-free on purpose: Session actions can import it without pulling in - * (and creating import cycles with) the modules that own the caches. + * This module must not import the modules that own the caches: Session actions import it, + * and such an import would create a cycle. */ function registerSessionCleanupCallback(callback: SessionCleanupCallback) { callbacks.push(callback); @@ -17,7 +19,12 @@ function registerSessionCleanupCallback(callback: SessionCleanupCallback) { /** Runs all registered cleanup callbacks. Called on sign-out. */ function runSessionCleanupCallbacks() { for (const callback of callbacks) { - callback(); + try { + callback(); + } catch (error) { + // A failing callback must not stop the rest — skipping a cleanup would leak the signed-out account's data into the next session. + Log.warn('[SessionCleanup] A cleanup callback threw an error', {error}); + } } } diff --git a/tests/unit/OptionsListUtilsTest.tsx b/tests/unit/OptionsListUtilsTest.tsx index 845dd72e7bb1..72c2ffcca2d5 100644 --- a/tests/unit/OptionsListUtilsTest.tsx +++ b/tests/unit/OptionsListUtilsTest.tsx @@ -8349,6 +8349,46 @@ describe('OptionsListUtils', () => { expect(result.personalDetails.length).toBeGreaterThan(0); }); + + it('should keep top-level fields of returned options mutable while the cached entry stays pristine', () => { + const first = createFilteredOptionList(PERSONAL_DETAILS, REPORTS, undefined, EMPTY_PRIVATE_IS_ARCHIVED_MAP, undefined); + const firstOption = first.personalDetails.at(0); + expect(firstOption).toBeDefined(); + if (firstOption) { + firstOption.isSelected = true; + } + + const second = createFilteredOptionList(PERSONAL_DETAILS, REPORTS, undefined, EMPTY_PRIVATE_IS_ARCHIVED_MAP, undefined); + const secondOption = second.personalDetails.at(0); + + // Same cache entry (nested objects are shared between clones), but each caller gets fresh top-level objects. + expect(secondOption?.icons).toBe(firstOption?.icons); + expect(secondOption).not.toBe(firstOption); + expect(secondOption?.isSelected).toBeFalsy(); + }); + + // The cached entry is frozen in dev, so a consumer that mutates a nested object shared with the + // cache throws instead of silently corrupting the results returned to every other screen. + it('should throw when a nested object shared with the cache is mutated', () => { + const result = createFilteredOptionList(PERSONAL_DETAILS, REPORTS, undefined, EMPTY_PRIVATE_IS_ARCHIVED_MAP, undefined); + const icons = result.personalDetails.at(0)?.icons; + + expect(icons?.length).toBeGreaterThan(0); + expect(() => icons?.pop()).toThrow(TypeError); + }); + + // Onyx snapshot objects referenced by the options are shared with the whole app and existing code + // still writes to them in place, so the dev freeze must leave them untouched (see deepFreeze). + it('should not freeze the Onyx snapshot objects referenced by cached options', () => { + const result = createFilteredOptionList(PERSONAL_DETAILS, REPORTS, undefined, EMPTY_PRIVATE_IS_ARCHIVED_MAP, undefined); + const personalDetailItem = result.personalDetails.at(0)?.item; + const reportItem = result.reports.at(0)?.item; + + expect(personalDetailItem).toBeDefined(); + expect(Object.isFrozen(personalDetailItem)).toBe(false); + expect(reportItem).toBeDefined(); + expect(Object.isFrozen(reportItem)).toBe(false); + }); }); describe('getFilteredRecentAttendees', () => { it('should deduplicate recent attendees by email', () => { diff --git a/tests/unit/SessionCleanupTest.ts b/tests/unit/SessionCleanupTest.ts new file mode 100644 index 000000000000..93d7c746bc53 --- /dev/null +++ b/tests/unit/SessionCleanupTest.ts @@ -0,0 +1,17 @@ +import Log from '@libs/Log'; +import {registerSessionCleanupCallback, runSessionCleanupCallbacks} from '@libs/SessionCleanup'; + +describe('SessionCleanup', () => { + it('runs the remaining callbacks when an earlier one throws', () => { + const logWarnSpy = jest.spyOn(Log, 'warn').mockImplementation(() => {}); + const laterCallback = jest.fn(); + registerSessionCleanupCallback(() => { + throw new Error('cleanup failed'); + }); + registerSessionCleanupCallback(laterCallback); + + expect(() => runSessionCleanupCallbacks()).not.toThrow(); + expect(laterCallback).toHaveBeenCalledTimes(1); + expect(logWarnSpy).toHaveBeenCalled(); + }); +}); diff --git a/tests/unit/usePrivateIsArchivedMapTest.ts b/tests/unit/usePrivateIsArchivedMapTest.ts index aaaae6695a29..6f8c66138845 100644 --- a/tests/unit/usePrivateIsArchivedMapTest.ts +++ b/tests/unit/usePrivateIsArchivedMapTest.ts @@ -71,6 +71,16 @@ describe('usePrivateIsArchivedMap', () => { expect(result.current[`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${REPORT_ID_1}`]).toBe(true); }); + it('returns a frozen map in dev so an accidental write throws instead of corrupting the shared cache', async () => { + await act(async () => { + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${REPORT_ID_1}`, {private_isArchived: ARCHIVED_DATE}); + }); + + const {result} = renderHook(() => usePrivateIsArchivedMap()); + + expect(Object.isFrozen(result.current)).toBe(true); + }); + it('handles a mix of archived and non-archived reports', async () => { await act(async () => { await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${REPORT_ID_1}`, {private_isArchived: ARCHIVED_DATE}); From 2aa2a51facdaa6d5a97e876a4ad73c82c66c2a87 Mon Sep 17 00:00:00 2001 From: sumo_slonik Date: Mon, 13 Jul 2026 13:04:40 +0200 Subject: [PATCH 3/4] Key the option-list cache on the sorted-actions derived value Replace the deprecatedReportActionsVersion counter (bumped inside the deprecated Onyx.connect callback) with the reference of sortedActions from the RAM_ONLY_SORTED_REPORT_ACTIONS derived value, which produces a new object on every recompute. useFilteredOptions now passes sortedActions down through processReport to createOption, so this call path also reads sorted actions from the derived value instead of the deprecated module-level map. --- src/hooks/useFilteredOptions.ts | 7 +++++++ src/libs/OptionsListUtils/index.ts | 30 ++++++++++++++++++++---------- 2 files changed, 27 insertions(+), 10 deletions(-) diff --git a/src/hooks/useFilteredOptions.ts b/src/hooks/useFilteredOptions.ts index 005b8edab812..91622e853db1 100644 --- a/src/hooks/useFilteredOptions.ts +++ b/src/hooks/useFilteredOptions.ts @@ -10,6 +10,7 @@ import useLocalize from './useLocalize'; import useOnyx from './useOnyx'; import usePrivateIsArchivedMap from './usePrivateIsArchivedMap'; import useReportAttributes from './useReportAttributes'; +import useSortedActions from './useSortedActions'; type UseFilteredOptionsConfig = { /** Maximum number of recent reports to pre-filter and process (default: 500). */ @@ -85,6 +86,10 @@ function useFilteredOptions(config: UseFilteredOptionsConfig = {}): UseFilteredO // Option building is locale-dependent, so a consumer that stays mounted through a language switch recomputes. const {preferredLocale} = useLocalize(); + // Sorted report actions from the RAM_ONLY_SORTED_REPORT_ACTIONS derived value; a new reference on + // every recompute, so it doubles as the report-actions invalidation signal for the option-list cache. + const sortedActions = useSortedActions(); + const privateIsArchivedMap = usePrivateIsArchivedMap(); const totalReports = allReports ? Object.keys(allReports).length : 0; @@ -109,6 +114,7 @@ function useFilteredOptions(config: UseFilteredOptionsConfig = {}): UseFilteredO undefined, undefined, isTrackIntentUser, + sortedActions, ) : null, [ @@ -124,6 +130,7 @@ function useFilteredOptions(config: UseFilteredOptionsConfig = {}): UseFilteredO deferContactsUntilSearch, preferredLocale, isTrackIntentUser, + sortedActions, ], ); diff --git a/src/libs/OptionsListUtils/index.ts b/src/libs/OptionsListUtils/index.ts index 8d4cb1a7e9ea..00521bc9afc4 100644 --- a/src/libs/OptionsListUtils/index.ts +++ b/src/libs/OptionsListUtils/index.ts @@ -251,16 +251,10 @@ const deprecatedCachedOneTransactionThreadReportIDs: Record; -// The sorted report-actions objects above are mutated in place (their references never change), -// so this version number is the only signal that their contents were updated. -let deprecatedReportActionsVersion = 0; - Onyx.connect({ key: ONYXKEYS.COLLECTION.REPORT_ACTIONS, waitForCollectionCallback: true, callback: (actions) => { - // Bump before the empty guard so clearing the collection also invalidates caches keyed on this version. - deprecatedReportActionsVersion++; if (!actions) { return; } @@ -1540,6 +1534,8 @@ function processReport( policyTags?: OnyxEntry, visibleReportActionsData: VisibleReportActionsDerivedValue = {}, isTrackIntentUser?: boolean, + // TODO: Remove optional (?) once all callers pass sortedActions. Refactor issue: https://github.com/Expensify/App/issues/66381 + sortedActions?: Record, ): { reportMapEntry?: [number, Report]; // The entry to add to reportMapForAccountIDs if applicable reportOption: SearchOption | null; // The report option to add to allReportOptions if applicable @@ -1563,7 +1559,18 @@ function processReport( reportMapEntry, reportOption: { item: report, - ...createOption({accountIDs, personalDetails, report, privateIsArchived, policy, reportAttributesDerived, policyTags, visibleReportActionsData, isTrackIntentUser}), + ...createOption({ + accountIDs, + personalDetails, + report, + privateIsArchived, + policy, + reportAttributesDerived, + policyTags, + visibleReportActionsData, + isTrackIntentUser, + sortedActions, + }), }, }; } @@ -1685,6 +1692,8 @@ function createFilteredOptionList( policyTags?: OnyxCollection, visibleReportActionsData: VisibleReportActionsDerivedValue = EMPTY_VISIBLE_REPORT_ACTIONS, isTrackIntentUser?: boolean, + // TODO: Remove optional (?) once all callers pass sortedActions. Refactor issue: https://github.com/Expensify/App/issues/66381 + sortedActions?: Record, ): OptionList { const {maxRecentReports = 500, includeP2P = true, isSearching = false, deferContactsUntilSearch = false, locale} = options; @@ -1710,9 +1719,9 @@ function createFilteredOptionList( isTrackIntentUser, // Option building translates strings imperatively (translateLocal), so the active locale is part of the output. locale ?? IntlStore.getCurrentLocale(), - // Option building reads module-level sorted report actions (getLastMessageTextForReport) that are - // mutated in place, so their version stands in for the never-changing object reference. - deprecatedReportActionsVersion, + // The RAM_ONLY_SORTED_REPORT_ACTIONS derived value produces a new object on every recompute, + // so its reference signals that the underlying report actions changed. + sortedActions, ]; const cachedEntry = shouldUseCache ? filteredOptionListCache.get(cacheEntryKey) : undefined; if (cachedEntry && cacheInputs.every((value, index) => value === cachedEntry.inputs.at(index))) { @@ -1775,6 +1784,7 @@ function createFilteredOptionList( reportPolicyTags, visibleReportActionsData, isTrackIntentUser, + sortedActions, ); if (reportMapEntry) { const [accountID, reportValue] = reportMapEntry; From c7816fe3ed704fff39249c12e0dbe7ba9ef65f03 Mon Sep 17 00:00:00 2001 From: sumo_slonik Date: Tue, 21 Jul 2026 09:47:33 +0200 Subject: [PATCH 4/4] Add test verifying cleanupSession runs registered session cleanup callbacks --- .../actions/SessionCleanupIntegrationTest.ts | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 tests/actions/SessionCleanupIntegrationTest.ts diff --git a/tests/actions/SessionCleanupIntegrationTest.ts b/tests/actions/SessionCleanupIntegrationTest.ts new file mode 100644 index 000000000000..abcdd3ad38f8 --- /dev/null +++ b/tests/actions/SessionCleanupIntegrationTest.ts @@ -0,0 +1,23 @@ +import {registerSessionCleanupCallback} from '@libs/SessionCleanup'; + +import {cleanupSession} from '@src/libs/actions/Session'; +import ONYXKEYS from '@src/ONYXKEYS'; + +import Onyx from 'react-native-onyx'; + +Onyx.init({ + keys: ONYXKEYS, +}); + +// Kept in its own file: cleanupSession has global side effects (clearCache, PersistedRequests.clear, ...) +// that break other tests when it runs inside a shared suite like SessionTest. +describe('Session cleanup integration', () => { + it('cleanupSession runs registered session cleanup callbacks', () => { + const callback = jest.fn(); + registerSessionCleanupCallback(callback); + + cleanupSession(); + + expect(callback).toHaveBeenCalledTimes(1); + }); +});