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.
|
melvin-bot
bot
requested review from
Krishna2323 and
joekaufmanexpensify
and removed request for
a team
|
@Krishna2323 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}
| isFocusedRef.current = isFocused; | ||
| }, [isFocused]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
❌ PERF-12 (docs)
The useEffect creates a setTimeout (line 569) but never returns a cleanup function to clear it. If newTransactionIDs changes before the timeout fires, or if the component unmounts, the stale timeout callback will still execute -- potentially calling scrollToIndex on an unmounted or stale carousel ref.
Return a cleanup function that clears the timeout:
useEffect(() => {
const index = carouselTransactions.findIndex((transaction) => newTransactionIDs?.has(transaction.transactionID));
if (index < 0) {
return;
}
const newTransaction = carouselTransactions.at(index);
const timeoutId = setTimeout(() => {
if (!isFocusedRef.current) {
return;
}
if (newTransaction?.transactionID !== carouselTransactionsRef.current.at(index)?.transactionID) {
return;
}
carouselRef.current?.scrollToIndex({
index,
viewOffset: -2 * styles.gap2.gap,
animated: true,
});
}, CONST.ANIMATED_TRANSITION);
return () => {
clearTimeout(timeoutId);
};
// We only want to scroll to a new transaction when the set of new transaction IDs changes.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [newTransactionIDs]);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Clearing the timeout breaks the logic. Since it's existing logic, I wouldn't touch it in this PR.
|
Same reassure test is failing in other PRs too: https://expensify.slack.com/archives/C01GTK53T8Q/p1774001704205909 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0469bf478b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const isFocused = useIsFocused(); | ||
| // We only want to highlight the new expenses if the screen is focused. | ||
| const newTransactionIDs = isFocused ? new Set(newTransactions.map((transaction) => transaction.transactionID)) : undefined; | ||
| const newTransactionIDs = new Set(newTransactions.map((transaction) => transaction.transactionID)); |
There was a problem hiding this comment.
newTransactionIDs is now created unconditionally, so TransactionPreview receives shouldHighlight=true even when the report screen is blurred but still mounted in the navigation stack. In that state the highlight animation can run off-screen and finish before the user returns, which means users can miss the “new expense” highlight entirely; this also contradicts the surrounding intent comment about only highlighting when focused. Keep the new scroll-timing fix, but reintroduce focus-gating for highlight IDs passed to preview items.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This was explained here: Expensify/react-native-onyx#724 (comment)
There was a problem hiding this comment.
In that state the highlight animation can run off-screen and finish before the user returns, which means users can miss the “new expense” highlight entirely
This is also mentioned in the Test steps (step 5), it highlights correctly on all of the platforms
src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx
Outdated
Show resolved
Hide resolved
|
No product review needed |
3 participants
Explanation of Change
This is the second apply on Onyx bump to v3.0.46.
Previous bump: #85248
Revert: #85767
It was reverted cause after merging another PR: #84622, several issues have appeared.
But they were not caused by the Onyx bump on their own (for more context check this slack thread - link).
@szymonzalarski98 is working on a fixed version of #84622.
Just to be safe, I'll include previously linked blockers to the test steps.
Bumps
react-native-onyxfrom 3.0.45 to 3.0.46 (Expensify/react-native-onyx#724 - Remove macro/microtasks during subscriber update).
This Onyx update changes subscriber notifications to be more synchronous, which exposed two issues in the App:
Side effect in render body —
updateLastAccessedWorkspace()was called directly during render in thewithPolicyHOC, causing "Cannot update a component while rendering a different component" errors. Wrapped inuseEffect. (context)New expense scroll/highlight race condition — Faster Onyx updates caused
useNewTransactionsto fire before navigation completed, soisFocusedwas stillfalseand new transactions were missed. Moved the focus check into the scrollsetTimeoutcallback via a ref, so it evaluates at scroll time rather than render time. (context)Fixed Issues
$ #82871
PROPOSAL: N/A
Tests
The E/App should work the same way as before. Let's verify basic test steps:
#85755
#85669
Offline tests
Same, as in the Tests section
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same, as in the Tests section
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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.mp4
Android: mWeb Chrome
android_web.mp4
iOS: Native
ios.mov
iOS: mWeb Safari
ios_web.mov
MacOS: Chrome / Safari
web.mp4