Await Onyx merge before native close in HybridApp#90913
Conversation
The closeReactNativeApp function was calling Navigation.clearPreloadedRoutes() and HybridAppModule.closeReactNativeApp() before the Onyx merge of closingReactNativeApp had resolved. This caused a race condition where React re-renders (e.g., StatusBar updates) could fire against a native surface already being torn down, leading to EXC_BAD_ACCESS crashes on iOS. Now the Onyx merge is awaited before clearing routes and calling the native module. A re-entry guard prevents double invocation during a single close sequence. Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
|
Fixed the failing ESLint check: the |
|
@MelvinBot |
|
PR has been marked as ready for review. 🤖 |
|
@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] |
Codecov Report✅ All modified and coverable lines are covered by tests. |
|
@MelvinBot |
|
Checklist updated. I checked off all items that are verifiable from code review or N/A for this PR (no new files, no CSS, no assets, no UI changes, no user-facing copy, no generic components, no Storybook, no deeplinks, no message editing code). Items left unchecked require human verification:
|
|
🚧 @amyevans has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
@MelvinBot |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreenrecorder-2026-05-19-11-49-21-802.mp4Android: mWeb ChromeiOS: HybridApp2026-05-19.11.52.54.mp4iOS: mWeb SafariMacOS: Chrome / Safari |
|
Added test cases to the PR description covering:
Also added matching QA steps. Note: the |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #90135 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| closingPromise.then(() => { | ||
| Navigation.clearPreloadedRoutes(); | ||
| // eslint-disable-next-line no-restricted-properties |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The eslint-disable-next-line no-restricted-properties comment lacks a justification explaining why this rule is being suppressed. Even though this comment was moved from the previous location, it should include an explanation for future maintainers.
Add a justification comment, for example:
// We need to call HybridAppModule.closeReactNativeApp directly as a native module method
// eslint-disable-next-line no-restricted-properties
HybridAppModule.closeReactNativeApp({shouldSetNVP});Reviewed at: cab5be6 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Fixed in 1410cc2 — added a justification comment explaining why no-restricted-properties is disabled for this native module call.
| HybridAppModule.closeReactNativeApp({shouldSetNVP}); | ||
| const closingPromise = CONFIG.IS_HYBRID_APP ? Onyx.merge(ONYXKEYS.HYBRID_APP, {closingReactNativeApp: true}) : Promise.resolve(); | ||
|
|
||
| closingPromise.then(() => { |
There was a problem hiding this comment.
❌ CONSISTENCY-6 (docs)
The .then() promise chain has no .catch() or error handler. If Onyx.merge rejects, the error will be silently swallowed as an unhandled promise rejection, and the native app close operation will never execute.
Add error handling to the promise chain:
closingPromise.then(() => {
Navigation.clearPreloadedRoutes();
// eslint-disable-next-line no-restricted-properties
HybridAppModule.closeReactNativeApp({shouldSetNVP});
}).catch((error) => {
Log.warn('Failed to merge HYBRID_APP closing state', {error});
// Still attempt to close the app to avoid leaving the user stuck
Navigation.clearPreloadedRoutes();
HybridAppModule.closeReactNativeApp({shouldSetNVP});
});Reviewed at: cab5be6 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Fixed in 1410cc2 — added a .catch() handler that logs a warning via Log.warn and still attempts to close the app so the user isn't left stuck.
|
@MelvinBot |
…andling - Added justification comment explaining why no-restricted-properties is disabled - Added .catch() handler to the promise chain so that if Onyx.merge rejects, the app still attempts to close rather than leaving the user stuck Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
|
@MelvinBot And podfile.lock But first update this branch to the latest version of main |
Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
|
Updated
|
|
@MelvinBot |
The patch reverted Onyx PR #770 (skippable collection member IDs). That revert was merged upstream in react-native-onyx PR #785 and is included in the 3.0.73 release this branch already uses, so the patch-package file is no longer needed. Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
|
Fixed the failing checks: removed the obsolete The patch was reverting Onyx PR #770, but that revert was merged upstream in Onyx PR #785 and is included in the 3.0.73 release this PR already upgrades to. The patch file was still targeting 3.0.71, causing |
…BeforeNativeClose Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
|
@MelvinBot |
…ub.com/Expensify/App into claude-awaitOnyxMergeBeforeNativeClose Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com> # Conflicts: # patches/react-native-onyx/details.md # patches/react-native-onyx/react-native-onyx+3.0.73.patch
|
Branch updated with the latest |
|
@MelvinBot |
|
MelvinBot has made 7 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
@MelvinBot |
…BeforeNativeClose Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com> # Conflicts: # patches/react-native-onyx/details.md # patches/react-native-onyx/react-native-onyx+3.0.73.patch
…eClose' into claude-awaitOnyxMergeBeforeNativeClose Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com> # Conflicts: # patches/react-native-onyx/details.md # patches/react-native-onyx/react-native-onyx+3.0.73.patch
|
Fixed the failing checks by merging Root cause: The branch had a commit ( The merge restores the |
|
There are no merge conflicts on this PR — it's cleanly mergeable with |
|
@MelvinBot |
Remove the original HybridApp await-before-close changes per reviewer request. This PR now only updates @shopify/react-native-skia from 2.4.14 to 2.6.2. Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
|
Done — reverted the HybridApp changes in ea951db. The PR now only contains the |
|
🚧 @dylanexpensify has triggered a test Expensify/App build. You can view the workflow run here. |
📲 Install BuildsAndroid
|
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
No crash on my side. Check please the video and let me know if I need something recheck. Thanks Screenrecording.05-21-2026.3-41-52.Pm.1.mp4 |
|
Unfortunately, I was able to reproduce it |
|
Thank you @ZhenjaHorbach! |
|
@MelvinBot |
|
Closed this PR as requested by |
5 participants
Explanation of Change
In HybridApp builds,
closeReactNativeAppwas firing three operations in rapid succession with no synchronization:Navigation.clearPreloadedRoutes(),Onyx.merge(closingReactNativeApp: true)(unawaited), andHybridAppModule.closeReactNativeApp(). This meant the native teardown could begin while Onyx-triggered React re-renders (e.g.,CustomStatusBarAndBackgroundupdating the status bar) were still in flight, causingEXC_BAD_ACCESScrashes on iOS when the re-render targeted an already-deallocated native view.This PR:
CustomStatusBarAndBackground) observeclosingReactNativeApp: trueand detach their listeners before native teardown begins.isClosingReactNativeApp) to prevent double invocation fromScreenWrapper'susePreventRemoveor repeated taps.Fixed Issues
$ #90135
PROPOSAL: #90135 (comment)
Tests
Precondition: These tests require a HybridApp (NewDot embedded in OldDot) iOS build. The crash is iOS-specific.
Verify close transition does not crash (main scenario)
closeReactNativeApp)EXC_BAD_ACCESSerrorVerify double-tap / rapid re-entry is prevented
Verify Onyx state resets correctly after re-opening
closingReactNativeAppis reset tofalseand does not block subsequent close operationsVerify no JS console errors
Offline tests
N/A - this change only affects the HybridApp close transition, not offline behavior.
QA Steps
EXC_BAD_ACCESScrash on iOS)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