Fix - Make it more apparent in the expense chat carousel view when expenses are added to a report#81446
Conversation
melvin-bot
bot
requested review from
dominictb and
trjExpensify
and removed request for
a team
|
@dominictb 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const viewabilityConfig = useMemo(() => { | ||
| return {itemVisiblePercentThreshold: 100}; | ||
| }, []); | ||
| const numberOfScrollToIndexFailed = useRef(0); |
There was a problem hiding this comment.
❌ PERFORMANCE-12 (docs)
The setTimeout call in onScrollToIndexFailed is not cleared when the component unmounts, which can cause memory leaks if the component is unmounted before the timeout fires.
Fix: Store the timeout ID and clear it in a cleanup function:
const timeoutRef = useRef<NodeJS.Timeout | null>(null);
const onScrollToIndexFailed = ({index}: {index: number; highestMeasuredFrameIndex: number; averageItemLength: number}) => {
if (numberOfScrollToIndexFailed.current > 4) {
return;
}
timeoutRef.current = setTimeout(() => {
carouselRef.current?.scrollToIndex({index, animated: true, viewOffset: 2 * styles.gap2.gap});
}, 100);
numberOfScrollToIndexFailed.current++;
};
useEffect(() => {
return () => {
if (timeoutRef.current) {
clearTimeout(timeoutRef.current);
}
};
}, []);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| if (index < 0) { | ||
| return; | ||
| } | ||
| const newTransaction = carouselTransactions.at(index); |
There was a problem hiding this comment.
❌ PERFORMANCE-12 (docs)
The setTimeout call inside useFocusEffect is not cleared when the component loses focus or unmounts, which can cause memory leaks and potential crashes if the component is unmounted before the timeout fires.
Fix: Return a cleanup function from the callback that clears the timeout:
useFocusEffect(
useCallback(() => {
const index = carouselTransactions.findIndex((transaction) => newTransactionIDs?.includes(transaction.transactionID));
if (index < 0) {
return;
}
const newTransaction = carouselTransactions.at(index);
const timeoutId = setTimeout(() => {
if (newTransaction?.transactionID !== carouselTransactionsRef.current.at(index)?.transactionID) {
return;
}
numberOfScrollToIndexFailed.current = 0;
carouselRef.current?.scrollToIndex({index, viewOffset: 2 * styles.gap2.gap, animated: true});
}, CONST.ANIMATED_TRANSITION);
return () => {
clearTimeout(timeoutId);
};
}, [newTransactionIDs]),
);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| carouselRef.current?.scrollToIndex({index, viewOffset: 2 * styles.gap2.gap, animated: true}); | ||
| }, CONST.ANIMATED_TRANSITION); | ||
|
|
||
| // eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The eslint-disable comment lacks a clear explanation of why the dependencies are intentionally omitted from the useFocusEffect dependency array.
Fix: Add a comment explaining why carouselTransactions is omitted:
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
// We only want to scroll when newTransactionIDs changes, not when carouselTransactions updatesPlease rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| } | ||
| }, [iouReportID, isSmallScreenWidth]); | ||
| const [reportMetadata] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${chatReportID}`, {canBeMissing: true}); | ||
| const newTransactions = useNewTransactions(reportMetadata?.hasOnceLoadedReportActions, transactions); |
There was a problem hiding this comment.
❌ PERF-13 (docs)
The .map() operation creates a new array on every render. While the array content may be the same, the array reference changes, which can trigger unnecessary re-renders in child components receiving newTransactionIDs as a prop.
Fix: Wrap in useMemo to maintain referential stability:
const newTransactions = useNewTransactions(reportMetadata?.hasOnceLoadedReportActions, transactions);
const newTransactionIDs = useMemo(() => newTransactions.map((transaction) => transaction.transactionID), [newTransactions]);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| }, [carouselTransactions]); | ||
|
|
||
| useFocusEffect( | ||
| useCallback(() => { |
There was a problem hiding this comment.
❌ PERF-2 (docs)
The findIndex callback performs an expensive operation (array lookup with .includes()) for every transaction without any early filtering. This could be optimized by converting newTransactionIDs to a Set for O(1) lookup instead of O(n) lookup with array .includes().
Fix: Convert to a Set for faster lookups:
useFocusEffect(
useCallback(() => {
const newTransactionIDsSet = new Set(newTransactionIDs);
const index = carouselTransactions.findIndex((transaction) => newTransactionIDsSet.has(transaction.transactionID));
if (index < 0) {
return;
}
// ... rest of the code
}, [newTransactionIDs]),
);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
|
||
| // Sometimes scrollToIndex might be called before the item is rendered so we will re-call scrollToIndex after a small delay. | ||
| setTimeout(() => { | ||
| carouselRef.current?.scrollToIndex({index, animated: true, viewOffset: 2 * styles.gap2.gap}); |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The hardcoded value 100 (milliseconds) is used as a setTimeout delay without clear documentation. This magic number reduces code readability.
Fix: Define a named constant or use an existing constant from CONST:
const SCROLL_TO_INDEX_RETRY_DELAY = 100; // milliseconds to wait before retrying scrollToIndex
const onScrollToIndexFailed = ({index}: {index: number; highestMeasuredFrameIndex: number; averageItemLength: number}) => {
if (numberOfScrollToIndexFailed.current > 4) {
return;
}
setTimeout(() => {
carouselRef.current?.scrollToIndex({index, animated: true, viewOffset: 2 * styles.gap2.gap});
}, SCROLL_TO_INDEX_RETRY_DELAY);
numberOfScrollToIndexFailed.current++;
};Or check if theres an appropriate constant in CONST that can be reused.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const onScrollToIndexFailed: (info: {index: number; highestMeasuredFrameIndex: number; averageItemLength: number}) => void = ({index}) => { | ||
| // There is a probability of infinite loop so we want to make sure that it is not called more than 5 times. | ||
| if (numberOfScrollToIndexFailed.current > 4) { | ||
| return; |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The hardcoded value 4 is used as a maximum retry limit without being defined as a named constant. This magic number reduces code readability and makes the intent less clear.
Fix: Define a named constant:
const MAX_SCROLL_TO_INDEX_RETRIES = 5; // Maximum number of retries before stopping
const onScrollToIndexFailed = ({index}: {index: number; highestMeasuredFrameIndex: number; averageItemLength: number}) => {
if (numberOfScrollToIndexFailed.current >= MAX_SCROLL_TO_INDEX_RETRIES) {
return;
}
// ... rest of the code
};Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| reportPreviewAction={action} | ||
| onPreviewPressed={openReportFromPreview} | ||
| shouldShowPayerAndReceiver={shouldShowPayerAndReceiver} | ||
| shouldHighlight={newTransactionIDs.includes(item.transactionID)} |
There was a problem hiding this comment.
❌ PERF-1 (docs)
While this is not a spread operator violation, using .includes() on an array inside renderItem is inefficient. The array lookup has O(n) complexity and runs for every item rendered in the list.
Fix: Convert newTransactionIDs to a Set before passing to child components:
In the parent component:
const newTransactions = useNewTransactions(reportMetadata?.hasOnceLoadedReportActions, transactions);
const newTransactionIDsSet = useMemo(() => new Set(newTransactions.map((transaction) => transaction.transactionID)), [newTransactions]);Then pass the Set and use .has():
shouldHighlight={newTransactionIDsSet.has(item.transactionID)}This changes the lookup from O(n) to O(1) for each rendered item.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
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.
|
|
Video seems good to me 👍 |
2 participants
Explanation of Change
Fixed Issues
We are fixing regression for the previous reverted pr
$ #71390 #78526
PROPOSAL: #71390 (comment)
Tests
Prerequisite: Account has at least one workspace.
Offline tests
Same as above
QA Steps
Same as above
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel so 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
2026-01-02.19-57-12.mp4
Android: mWeb Chrome
2026-01-02.21-59-37.mp4
iOS: Native
2026-01-02.20-39-38.mp4
iOS: mWeb Safari
2026-01-02.21-58-45.mp4
MacOS: Chrome / Safari
2026-02-05.00-41-16.mp4