perf: Add NON_PERSONAL_AND_WORKSPACE_CARD_LIST derived value#79347
Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
LukasMod
force-pushed
the
perf/open-report-derived-cards
branch
2 times, most recently
from
eae2aa8 to
f8a0987
Compare
LukasMod
force-pushed
the
perf/open-report-derived-cards
branch
from
f78f4a6 to
ecb57a7
Compare
LukasMod
changed the title
LukasMod
force-pushed
the
perf/open-report-derived-cards
branch
from
ecb57a7 to
9f17e50
Compare
…to perf/open-report-derived-cards
melvin-bot
Bot
requested review from
heyjennahay and
situchan
and removed request for
a team and
heyjennahay
| const taxRates = useMemo(() => getAllTaxRates(policies), [policies]); | ||
| const allCards = useMemo(() => mergeCardListWithWorkspaceFeeds(workspaceCardFeeds ?? CONST.EMPTY_OBJECT, userCardList), [workspaceCardFeeds, userCardList]); |
There was a problem hiding this comment.
Are you sure that useMemo really improves performance?
Not sure why React compiler doesn't handle it already.
There was a problem hiding this comment.
I did few tests with it, from compiler side it looks like that:
entry code:
const taxRatesMemo = useMemo(() => getAllTaxRates(policies), [policies]);
const taxRates = getAllTaxRates(policies);
result:
if (
$[7] !== allReports ||
$[8] !== personalDetails ||
$[9] !== policies ||
$[10] !== policyCategories ||
$[11] !== policyTagsLists ||
$[12] !== queryJSON ||
$[13] !== t7 ||
$[14] !== userCardList ||
$[15] !== workspaceCardFeeds
) {
const currencyList = t7 === undefined ? getEmptyObject() : t7;
// Memoized version (compiled from useMemo([policies]))
let t9;
if ($[20] !== policies) {
t9 = getAllTaxRates(policies);
$[20] = policies;
$[21] = t9;
} else {
t9 = $[21];
}
taxRatesMemo = t9;
// Non-memoized version (recomputed on every region re-run)
taxRates = getAllTaxRates(policies);
}
taxRates recomputes every time something in the region is outdated, but taxRatesMemo recomputes only when additionally policies are outdated, which makes that difference
There was a problem hiding this comment.
I also added some logs inside mergeCardListWithWorkspaceFeeds to compare how many times it fires from that hook and there is a difference (same cause)
There was a problem hiding this comment.
I checked with logs again and
using just
const taxRates = getAllTaxRates(policies);
const allCards = mergeCardListWithWorkspaceFeeds(workspaceCardFeeds ?? CONST.EMPTY_OBJECT, userCardList);
generated 2x of each execution, when opening report. With useMemo its 0. Here is source output with and without memo:
useFIlterFormValues - NO MEMO.txt
useFIlterFormValues - MEMO.txt
There was a problem hiding this comment.
Interesting, not sure why React Compiler seems to not memoize most ideally in this case. Maybe this is a bug or React Compiler just decided, that the function is cheap to execute and therefore skips optimization.
@LukasMod do you have a working repro for this so we could investigate this further and potentially create an upstream issue?
also cc @roryabraham
There was a problem hiding this comment.
I added comment as @chrispader suggested here https://callstack-hq.slack.com/archives/C05LX9D6E07/p1768910560155529?thread_ts=1768850680.184809&cid=C05LX9D6E07
There was a problem hiding this comment.
@chrispader You can just take useFilterFormValues and remove that useMemo and check how source files looks like. From what I understand from output, compiler creates area for returned value formValues and its all dependencies.
For useMemo compiler emits three separate memoized regions:
taxRatesmemoized on[policies]allCardsmemoized on[(workspaceCardFeeds ?? CONST.EMPTY_OBJECT), userCardList]formValuesmemoized on[allCards, allReports, currencyList, personalDetails, policyCategories, policyTagsLists, queryJSON, taxRates]
So each derived value can be reused independently.
Without useMemo compiler emits one single memoized region around the entire derivation:
formValuesmemoized on[allReports, currencyList, personalDetails, policies, policyCategories, policyTagsLists, queryJSON, userCardList, workspaceCardFeeds]
Inside that one region it recomputes taxRates and allCards together, whenever any dependency changes.
Updated files from latest main, you can see changes in region condition:
useFIlterFormValues.-.MEMO.txt
useFIlterFormValues.-.NO.MEMO.txt
useFilterFormValues for testing (just check with useMemo and without):
import {useMemo} from 'react';
import {usePersonalDetails} from '@components/OnyxListItemProvider';
import type {SearchQueryJSON} from '@components/Search/types';
import {mergeCardListWithWorkspaceFeeds} from '@libs/CardUtils';
import {getAllTaxRates} from '@libs/PolicyUtils';
import {buildFilterFormValuesFromQuery} from '@libs/SearchQueryUtils';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type {SearchAdvancedFiltersForm} from '@src/types/form';
import {getEmptyObject} from '@src/types/utils/EmptyObject';
import useCurrencyList from './useCurrencyList';
import useOnyx from './useOnyx';
const useFilterFormValues = (queryJSON?: SearchQueryJSON) => {
const personalDetails = usePersonalDetails();
const {currencyList} = useCurrencyList();
const [userCardList] = useOnyx(ONYXKEYS.CARD_LIST, {canBeMissing: true});
const [policies] = useOnyx(ONYXKEYS.COLLECTION.POLICY, {canBeMissing: true});
const [allReports] = useOnyx(ONYXKEYS.COLLECTION.REPORT, {canBeMissing: true});
const [policyTagsLists] = useOnyx(ONYXKEYS.COLLECTION.POLICY_TAGS, {canBeMissing: true});
const [policyCategories] = useOnyx(ONYXKEYS.COLLECTION.POLICY_CATEGORIES, {canBeMissing: true});
const [workspaceCardFeeds] = useOnyx(ONYXKEYS.COLLECTION.WORKSPACE_CARDS_LIST, {canBeMissing: true});
// Helps to avoid unnecessary recalculations when user open report details screen. React Compiler does not provide same result.
const taxRates = useMemo(() => getAllTaxRates(policies, 'useMemo'), [policies]);
const allCards = useMemo(() => mergeCardListWithWorkspaceFeeds(workspaceCardFeeds ?? CONST.EMPTY_OBJECT, userCardList, false, 'useMemo'), [workspaceCardFeeds, userCardList]);
// const taxRates = getAllTaxRates(policies, 'compiler')
// const allCards = mergeCardListWithWorkspaceFeeds(workspaceCardFeeds ?? CONST.EMPTY_OBJECT, userCardList, false, 'compiler');
const formValues = queryJSON
? buildFilterFormValuesFromQuery(queryJSON, policyCategories, policyTagsLists, currencyList, personalDetails, allCards, allReports, taxRates)
: getEmptyObject<Partial<SearchAdvancedFiltersForm>>();
return formValues;
};
export default useFilterFormValues;
…to perf/open-report-derived-cards
…to perf/open-report-derived-cards
|
Please fix conflict |
…to perf/open-report-derived-cards
|
@situchan Looks like conflicting PR was reverted. I updated from main anyway |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-01-25.at.11.29.41.PM.mov |
Btw no test cases added |
| const taxRates = useMemo(() => getAllTaxRates(policies), [policies]); | ||
| const allCards = useMemo(() => mergeCardListWithWorkspaceFeeds(workspaceCardFeeds ?? CONST.EMPTY_OBJECT, userCardList), [workspaceCardFeeds, userCardList]); |
|
@mountiny I added additional unit tests and did some renaming as requested 👍 |
|
@LukasMod Thank you! Sorry there are new conflicts again 😢 |
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Really appreciate you adding the tests, I think that makes me much more confident in the change
…to perf/open-report-derived-cards
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.3.11-0 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.3.12-1 🚀
|
5 participants
Explanation of Change
NON_PERSONAL_AND_WORKSPACE_CARD_LISTwith filtering adjusted to most common caseuseFilterFormValueswhere, even though the file is compiled with the React Compiler, addinguseMemo()reduces a few retriggers each time a report is openedFixed Issues
$ #79670
PROPOSAL:
Tests
Prerequisites
Steps
Offline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android.native.mov
Android: mWeb Chrome
android.web.mov
iOS: Native
ios.native.mov
iOS: mWeb Safari
ios.web.mov
MacOS: Chrome / Safari
web.chrome.mov