Reduce re-renders of MoneyRequestReportPreview#64940
Merged
mountiny merged 10 commits intoExpensify:mainfrom Jul 17, 2025
Merged
Reduce re-renders of MoneyRequestReportPreview#64940mountiny merged 10 commits intoExpensify:mainfrom
mountiny merged 10 commits intoExpensify:mainfrom
Conversation
zirgulis
force-pushed
the
fix/reduce-state-updates-in-onlayout
branch
from
2a0b01c to
95b17c5
Compare
Contributor
|
@zirgulis Can you please make this ready for a review and fix the unit tests? |
…to fix/reduce-state-updates-in-onlayout
Contributor
|
@zirgulis Lets please prioritize making this ready for a review, thanks! |
zirgulis
changed the title
|
@hoangzinh Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Contributor
|
@zirgulis Is there any way that we can measure performance to prove it has been reduced? |
Contributor
Author
|
@hoangzinh yes the testing steps are mentioned in the |
hoangzinh
reviewed
Jul 9, 2025
Contributor
There was a problem hiding this comment.
One concern here is: it won't trigger recalculate reportPreviewStyles when layout is changed again (like change window size, or vertical mode -> horizontal mode)
Contributor
There was a problem hiding this comment.
But I couldn't find a regression issue related to this concern.
Screen.Recording.2025-07-10.at.09.04.16.mov
Contributor
Author
There was a problem hiding this comment.
@hoangzinh I managed to find a regression: when using the responsive window size in the browser the styles recalculation was not working properly on mobile dimensions, it was showing many smaller TransactionPreview components instead of one big TransactionPreview. I.e. the recalculation of transactionPreviewCarouselStyle was not working properly:
Screen.Recording.2025-07-10.at.18.19.45.mov
I have fixed that in my latest commit , now it correctly recalculates the TransactionPreview "box" while still reducing state updates when not resizing:
Screen.Recording.2025-07-10.at.18.20.28.mov
…to fix/reduce-state-updates-in-onlayout
hoangzinh
reviewed
Jul 10, 2025
Contributor
There was a problem hiding this comment.
I don't feel strong about this trick, tbh. It will trigger re-render 2 times in the narrow layout.
Wdyt about this?
const widthsRef = useRef({currentWidth: null, currentWrapperWidth: null});
const [widths, setWidths] = useState({currentWidth: 0, currentWrapperWidth: 0});
const updateWidths = () => {
const {currentWidth, currentWrapperWidth} = widthsRef.current;
if (currentWidth !== null && currentWrapperWidth !== null) {
setWidths({currentWidth, currentWrapperWidth});
widthRefs.current = {};
}
};
const onCarouselLayout = (e) => {
widthsRef.current.currentWidth = e.nativeEvent.layout.width;
updateWidths();
};
const onWrapperLayout = (e) => {
widthsRef.current.currentWrapperWidth = e.nativeEvent.layout.width;
updateWidths();
};
// Then update existing reportPreviewStyles to
const reportPreviewStyles = useMemo(
() => StyleUtils.getMoneyRequestReportPreviewStyle(shouldUseNarrowLayout, transactions.length, widths.currentWidth, widths.currentWrapperWidth),
[StyleUtils, widths, shouldUseNarrowLayout, transactions.length],
);
Contributor
Author
There was a problem hiding this comment.
can you provide the whole code? It's unclear to me how your code should work, e.g. widths state is not used anywhere
Contributor
There was a problem hiding this comment.
Updated the above code suggestion. Can you take a look again? My idea is update them in a batch
Contributor
Author
There was a problem hiding this comment.
Contributor
There was a problem hiding this comment.
Hmm, sorry, I didn't test it carefully. I just updated it. Anw, it's just my suggestion. Feel free to choose any solution that works for you first.
diff --git a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx
index 321f4ad0a7a..8c8510508fb 100644
--- a/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx
+++ b/src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx
@@ -1,4 +1,4 @@
-import React, {useCallback, useMemo, useState} from 'react';
+import React, {useCallback, useMemo, useRef, useState} from 'react';
import type {LayoutChangeEvent, ListRenderItem} from 'react-native';
import {usePersonalDetails} from '@components/OnyxProvider';
import TransactionPreview from '@components/ReportActionItem/TransactionPreview';
@@ -52,12 +52,31 @@ function MoneyRequestReportPreview({
const lastTransactionViolations = useTransactionViolations(lastTransaction?.transactionID);
const isTrackExpenseAction = isTrackExpenseActionReportActionsUtils(action);
const isSplitBillAction = isSplitBillActionReportActionsUtils(action);
- const [currentWidth, setCurrentWidth] = useState<number>(0);
- const [currentWrapperWidth, setCurrentWrapperWidth] = useState<number>(0);
+
+ const widthsRef = useRef({currentWidth: 0, currentWrapperWidth: 0});
+ const [widths, setWidths] = useState({currentWidth: 0, currentWrapperWidth: 0});
+
const reportPreviewStyles = useMemo(
- () => StyleUtils.getMoneyRequestReportPreviewStyle(shouldUseNarrowLayout, transactions.length, currentWidth, currentWrapperWidth),
- [StyleUtils, currentWidth, currentWrapperWidth, shouldUseNarrowLayout, transactions.length],
+ () => StyleUtils.getMoneyRequestReportPreviewStyle(shouldUseNarrowLayout, transactions.length, widths.currentWidth, widths.currentWrapperWidth),
+ [StyleUtils, widths, shouldUseNarrowLayout, transactions.length],
);
+
+ const updateWidths = () => {
+ const {currentWidth, currentWrapperWidth} = widthsRef.current;
+ if (currentWidth !== 0 && currentWrapperWidth !== 0) {
+ setWidths({currentWidth, currentWrapperWidth});
+ }
+ };
+
+ const onCarouselLayout = (e) => {
+ widthsRef.current.currentWidth = e.nativeEvent.layout.width;
+ updateWidths();
+ };
+ const onWrapperLayout = (e) => {
+ widthsRef.current.currentWrapperWidth = e.nativeEvent.layout.width;
+ updateWidths();
+ };
+
const shouldShowPayerAndReceiver = useMemo(() => {
if (!isIOUReport(iouReport) && action.childType !== CONST.REPORT.TYPE.IOU) {
return false;
@@ -121,13 +140,9 @@ function MoneyRequestReportPreview({
invoiceReceiverPolicy={invoiceReceiverPolicy}
lastTransactionViolations={lastTransactionViolations}
renderTransactionItem={renderItem}
- onCarouselLayout={(e: LayoutChangeEvent) => {
- setCurrentWidth(e.nativeEvent.layout.width);
- }}
- onWrapperLayout={(e: LayoutChangeEvent) => {
- setCurrentWrapperWidth(e.nativeEvent.layout.width);
- }}
- currentWidth={currentWidth}
+ onCarouselLayout={onCarouselLayout}
+ onWrapperLayout={onWrapperLayout}
+ currentWidth={widthsRef.current.currentWidth}
reportPreviewStyles={reportPreviewStyles}
shouldDisplayContextMenu={shouldDisplayContextMenu}
isInvoice={isInvoice}
Contributor
Author
There was a problem hiding this comment.
@hoangzinh please have another look I fixed your POC + added some optimizations
Contributor
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-07-11.at.23.25.32.movAndroid: mWeb ChromeScreen.Recording.2025-07-11.at.23.05.30.android.chrome.moviOS: HybridAppScreen.Recording.2025-07-11.at.23.17.35.ios.moviOS: mWeb SafariScreen.Recording.2025-07-11.at.23.11.53.ios.safari.movMacOS: Chrome / SafariScreen.Recording.2025-07-11.at.22.32.23.web.movMacOS: DesktopScreen.Recording.2025-07-11.at.22.54.27.desktop.mov |
Contributor
Author
|
I'm OOO for today, will address any upcoming PR comments tomorrow. |
Contributor
|
Also adding @Guccio163 for a review kindly, thanks |
Contributor
|
Code-wise it looks okay, but I think something is still buggy, because 'iOS: mWeb Safari' in @hoangzinh 's Reviewer Checklist looks wrong on 0:09 - Carousel doesn't scroll right on the dot click; It did occur to me some time ago during the development of the Carousel when TransactionPreview width was calculated wrong or value which it operates on isn't updated properly, you should check it 🔎 |
Contributor
|
Thank you @Guccio163. @zirgulis, can you take a look to see if that bug is caused by this PR? |
…to fix/reduce-state-updates-in-onlayout
Contributor
Author
|
@hoangzinh @Guccio163 I can't reproduce that bug. What are the steps to reproduce this? Screen.Recording.2025-07-15.at.19.32.37.mov |
Contributor
|
I couldn't reproduce it again, either Screen.Recording.2025-07-16.at.04.43.05.mov |
Contributor
|
@Guccio163 It seems to be an intermittent bug. Anw, I don't think this PR causes that bug. Can we go ahead with this PR? |
Contributor
|
@hoangzinh maybe its repro on main too |
Contributor
|
I couldn't reproduce again, so I can't verify @mountiny. I think we can go ahead with the PR. If we catch this bug later with reproducible steps, we can fix it later. |
72 tasks
Contributor
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.83-0 🚀
|
This was referenced Jul 18, 2025
Contributor
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.1.83-5 🚀
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
Reduce rerenders of MoneyRequestReportPreview by batching all measurements using refs and trigger a single re-render only when all required dimensions are collected.
Fixed Issues
$ #65524
Tests
mainbranch and compare the results.Offline tests
QA Steps
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))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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop