[CP Staging] fix: Downgrade Onyx to v3.0.45#85767
Conversation
|
|
chrispader
changed the title
chrispader
changed the title
melvin-bot
bot
requested review from
ZhenjaHorbach and
joekaufmanexpensify
and removed request for
a team
|
@ZhenjaHorbach 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] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari2026-03-19.12.48.27.mov2026-03-19.12.52.12.mov |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (!policyID) { | ||
| return; | ||
| } | ||
| if (policyID && policyID.length > 0) { |
There was a problem hiding this comment.
❌ PERF-8 (docs)
updateLastAccessedWorkspace(policyID) is a side effect (Onyx.set) called directly in the component render body. This will execute on every render, including re-renders caused by unrelated state or prop changes, React Strict Mode double-invocations, and concurrent rendering retries. The previous useEffect approach was correct because it limited execution to when policyID actually changed.
Restore the useEffect wrapper:
useEffect(() => {
if (!policyID) {
return;
}
updateLastAccessedWorkspace(policyID);
}, [policyID]);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
|
||
| const isFocused = useIsFocused(); | ||
| const isFocusedRef = useRef(isFocused); | ||
| useFocusEffect( |
There was a problem hiding this comment.
❌ PERF-12 (docs)
The setTimeout inside the useFocusEffect callback has no cleanup mechanism. If the screen loses focus or the component unmounts before CONST.ANIMATED_TRANSITION elapses, the timer callback will still fire, potentially calling scrollToIndex on an unmounted or stale ref. The previous implementation guarded against this with isFocusedRef.current, but that guard has been removed.
Return a cleanup function from the useFocusEffect callback to clear the timer:
useFocusEffect(
useCallback(() => {
const index = carouselTransactions.findIndex(...);
if (index < 0) {
return;
}
const newTransaction = carouselTransactions.at(index);
const timeoutId = setTimeout(() => {
if (newTransaction?.transactionID !== carouselTransactionsRef.current.at(index)?.transactionID) {
return;
}
carouselRef.current?.scrollToIndex({ ... });
}, CONST.ANIMATED_TRANSITION);
return () => clearTimeout(timeoutId);
// 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.
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [newTransactionIDs]), | ||
| ); | ||
|
|
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The eslint-disable-next-line react-hooks/exhaustive-deps comment lacks a justification explaining why the dependencies are intentionally omitted (e.g., carouselTransactions and styles are used in the callback but not listed).
Add an explanatory comment before the disable:
// We only want to scroll to a new transaction when the set of new transaction IDs changes.
// eslint-disable-next-line react-hooks/exhaustive-depsPlease rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
mountiny
changed the title
chrispader
changed the title
chrispader
changed the title
|
@mountiny But probably we have no choice |
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
![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: 872a52a112
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const isFocused = useIsFocused(); | ||
| // We only want to highlight the new expenses if the screen is focused. | ||
| const newTransactionIDs = new Set(newTransactions.map((transaction) => transaction.transactionID)); | ||
| const newTransactionIDs = isFocused ? new Set(newTransactions.map((transaction) => transaction.transactionID)) : undefined; |
There was a problem hiding this comment.
useNewTransactions() only reports additions on the render where the transaction list grows (src/hooks/useNewTransactions.ts:17-26). By replacing the set with undefined whenever useIsFocused() is false, a transaction that arrives while this preview is mounted in the background is dropped on that one render, and the next focus render already treats the updated list as the baseline. In practice, expenses created while another screen is on top will never be highlighted or auto-scrolled when the user returns to this preview.
Useful? React with 👍 / 👎.
| setTimeout(() => { | ||
| // If the new transaction is not available at the index it was on before the delay, avoid the scrolling | ||
| // because we are scrolling to either a wrong or unavailable transaction (which can cause crash). | ||
| if (newTransaction?.transactionID !== carouselTransactionsRef.current.at(index)?.transactionID) { | ||
| return; |
There was a problem hiding this comment.
This setTimeout() now survives a blur because useFocusEffect is not returning a cleanup and the previous isFocusedRef.current check was removed. If the user navigates away within CONST.ANIMATED_TRANSITION after a new expense arrives, stack navigators that keep the old screen mounted will still execute scrollToIndex() against the hidden carousel, so coming back later can land on an unexpected item. The earlier implementation explicitly skipped this delayed scroll once the screen lost focus.
Useful? React with 👍 / 👎.
But never mind |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
This comment has been minimized.
This comment has been minimized.
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
LGTM, asked QA for a retest of the blocker
…3.0.45 [CP Staging] fix: Downgrade Onyx to v3.0.45 (cherry picked from commit 5277b58) (cherry-picked to staging by mountiny)
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.3.40-5 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.3.40-10 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.3.41-0 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.41-4 🚀
|
5 participants
@mountiny
Explanation of Change
This is one of two PRs which revert recent Onyx bumps that cause deploy blockers related to issues with attachment/receipt upload
This reverts the Onyx bump from version 3.0.45 to 3.0.46.
Fixed Issues
$ #85755
$ #85669
PROPOSAL:
Tests
Offline tests
QA Steps
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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari