chore: Bump Onyx to version 3.0.63 to fix memory leaks and other issues in NitroSQLite and Nitro#84517
Conversation
|
|
chrispader
changed the title
3.0.46 to fix memory leaks and other issues in NitroSQLite and Nitro3.0.46 to fix memory leaks and other issues in NitroSQLite and Nitro
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
Notification - No notification is displayed when receiving message in chatsVersion Number: 84519 Action Performed:
Expected Result:Chat with account B is marked as unread (bold) and Notification is sent in the device Actual Result:Chat is marked as bold but no notification is sent in the device Workaround:Unknown Platforms:
Screenshots/VideosBug7098479_1773111576663.expensifial_account.mp4 |
Unread state doesn't display on the app iconVersion Number: 9.3.33-1 PR:84517 Ad-Hoc Action Performed:
Expected Result:Unread state should display on the app icon Actual Result:Unread state doesn't display on the app icon Workaround:Unknown Platforms:
Screenshots/VideosBug7098479_1773111576663.expensifial_account.mp4 |
|
@jponikarchuk looking at the above two issues, this seems unrelated to the changes made in this PR. Both notifications and app banners/indicators depend on permissions and how the app was installed and set up. Could you please verify if re-installing the app with proper permissions fixes these issues? |
|
Both issues are still reproducible when re-installing the app with proper permissions fixes Screen_Recording_20260311_142810.mp4
|
|
@mountiny Regression is completed Logged issues |
heyjennahay
left a comment
heyjennahay
left a comment
There was a problem hiding this comment.
Product review not required
|
Are external/expert contributors able to test push notifications on simulators/physical devices at this point? @mountiny @AndrewGable i remember there was this issue, to allow that but it's closed now. iOS simulators technically now support testing push notifications, but i wasn't able to get it working locally. Are there any steps that i need to do in order to test these? |
|
@mountiny also i'll merge main and resolve conflicts and maybe we can just trigger another ad-hoc build to verify that the issues mentioned above are actually caused by the changes here. They don't feel really related. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb Safarimsafari.movMacOS: Chrome / Safariweb.mov |
PR Review: Bump react-native-onyx to 3.0.63All CI checks pass. The Three items to flag: 1. Empty
|
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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 [isLoadingReportData] = useOnyx(ONYXKEYS.IS_LOADING_REPORT_DATA); | ||
| const [betas] = useOnyx(ONYXKEYS.BETAS); | ||
| const [reportActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${props.route.params.reportID}`, {canEvict: false}); | ||
| const [reportActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${props.route.params.reportID}`, {}); |
There was a problem hiding this comment.
NAB: we can remove empty json {} in 2nd param. 12 occurrences
There was a problem hiding this comment.
@fabioh8010 i was also wondering about this. Do we need these or can we remove?
There was a problem hiding this comment.
Removed all empty useOnyx options objects across the codebase!
| // Increase the cached key count so that the app works more consistently for accounts with large numbers of reports | ||
| maxCachedKeysCount: 50000, |
There was a problem hiding this comment.
Appreciate your brief context about this removal
There was a problem hiding this comment.
@situchan it was removed as part of Expensify/react-native-onyx#766. This number was so high that cache eviction system was never being triggered for any account, which was basically dead code and for that and other reasons we decided to remove from Onyx.
|
Please also prepare Mobile-Expensify PR to update Podfile.lock with NitroModules, RNNitroSQLite version bumps |
@situchan it's already linked in the PR description: |
|
Android build crashes on start. Other platforms work fine. |
|
@chrispader you have lint errors |
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
@chrispader @mountiny merged main to resolve this ESLint check fail. The issue was on main, no errors in the PR so the ad-hoc build is also fine |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
mountiny
changed the title
3.0.63 to fix memory leaks and other issues in NitroSQLite and Nitro3.0.63 to fix memory leaks and other issues in NitroSQLite and Nitro
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
LGTM, thank you! ready to merge after next app deploy
mountiny
changed the title
3.0.63 to fix memory leaks and other issues in NitroSQLite and Nitro3.0.63 to fix memory leaks and other issues in NitroSQLite and Nitro
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Not on hold anymore so going to merge
|
🚧 @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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.3.62-5 🚀
Bundle Size Analysis (Sentry): |
10 participants
@mountiny
Warning
If you need to revert this PR, revert https://github.com/Expensify/Mobile-Expensify/pull/13882 as well
Explanation of Change
Bumps
react-native-onyxto version 3.0.63.Version 3.0.62 includes updates to NitroSQLite and Nitro Modules. The latest version of NitroSQLite includes a fix for a memory leak issue in Nitro Modules. There are also other improvements made in NitroSQLite, that are explained in Expensify/react-native-onyx#749. (Details in recent release notes since version 9.2.0)
The memory leak issue on Android has been reported by @mateuuszzzzz in the linked issue. Comments:
Version 3.0.63 includes changes from #86736 by @fabioh8010
Fixed Issues
$ #63979
$ #85252
PROPOSAL:
MOBILE-EXPENSIFY: https://github.com/Expensify/Mobile-Expensify/pull/13882
Tests
Perform general tests which involve data being set to storage.
Offline tests
None needed.
QA Steps
Make sure that there are not errors or warnings reported in the console
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