Repository navigation
Fix iOS crash when returning from Settings after enabling Contacts access - #97992
Conversation
…n crash Co-authored-by: dmkt9 <dmkt9@users.noreply.github.com>
Co-authored-by: dmkt9 <dmkt9@users.noreply.github.com>
|
The failing checks were not caused by this PR's code — they were a transient GitHub Actions infrastructure outage. I re-triggered CI with an empty commit. What failed: EvidenceThe only remaining reds — The re-run is in progress on |
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.
|
|
✅ All the code checks are green now. The failures were a transient GitHub Actions outage, not a problem with this PR — the earlier re-trigger fixed them. On the latest commit, The only two remaining red checks are |
|
@MelvinBot update your checklist:
|
|
Updated the checklist as requested:
|
|
@MelvinBot mark this PR as ready |
|
Done — marked this PR as ready for review. ✅ |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.hybrid.mp4Android: mWeb ChromeiOS: HybridAppios.hybrid.mp4iOS: mWeb SafariMacOS: Chrome / Safari |
|
The only failing check is Every real CI check is green —
Next step: get an independent reviewer to approve the PR and this check will clear on its own. |
| } | ||
| updateLastRoute(''); | ||
| Navigation.navigate(lastRoute as Route); | ||
| // On iOS, changing the Contacts permission in Settings forces the app to reload (see `goToSettings`). |
There was a problem hiding this comment.
| // On iOS, changing the Contacts permission in Settings forces the app to reload (see `goToSettings`). | |
| // On iOS, changing the Contacts permission in Settings forces the app to reload (see `goToSettings`). |
There was a problem hiding this comment.
Applied Beamanator's suggestion — added a blank line to separate updateLastRoute('') from the comment block. Pushed in 2f1f27e.
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product.
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
|
The only failing check on this PR is That check isn't a code failure I can fix by pushing a commit. It's a governance gate: it fails until the PR receives an independent approval from a reviewer who isn't the author. There's no code change that clears it. To resolve: get an independent reviewer to approve the PR (the assigned reviewer / a Contributor+), and the check will pass. If you're seeing a different check fail, note the latest CI run was still in progress when I looked — re-tag me once it settles and I'll dig into any real failures. |
|
@Beamanator All yours |
|
🚧 Beamanator 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/Beamanator in version: 9.4.52-0 🚀
|
|
🤖 No help site changes required. I reviewed the change in this PR. It's a purely internal crash fix — There is no user-facing change here:
Because nothing customer-facing changed, no help site article needs updating, so I did not create a docs PR. @dmkt9, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.4.52-11 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
On iOS, tapping Import contacts sends the user to device Settings; when the Contacts permission changes, iOS cold-reloads the app (see
goToSettings). On relaunch,Expensify.tsxrestores the persisted deep route (lastRoute) — here the money-request participant selector, an RHP route — by callingNavigation.navigate(lastRoute)synchronously inside auseLayoutEffecton the boot frame.Restoring a deep RHP route on that same frame kicks the
@react-navigation/stackcard's legacyAnimatednative-driver entering transition while the tree is still hydrating. When the card unmounts mid-transition, the native animation's.start()→findNodeHandleruns against an already-unmounted node and throws "Unable to find node on an unmounted component", crashing the app. This matches the iOS crash stack reported on the issue (start → __startAnimationIfNative → __makeNative → _connectAnimatedView → findNodeHandle).The fix defers the route restore by one frame with
requestAnimationFrame, so the restore no longer races the card's mount-then-unmount within the boot frame.cancelAnimationFrameis called in the effect cleanup to avoid a stray navigate if the effect re-runs.requestAnimationFrameis used rather thanInteractionManager.runAfterInteractions(which is deprecated in our bundled React Native — see #71913) per the approved proposal.Fixed Issues
$ #97939
PROPOSAL: #97939 (comment)
Tests
Same as QA steps
Offline tests
Same as QA steps
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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