-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Migrate from withOnyx to useOnyx in AuthScreens #66093
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate from withOnyx to useOnyx in AuthScreens #66093
Conversation
|
I planned on opening this draft PR in a day or two but, since some progress has already been made, I thought it'd be better to document it here. First bug I found was an infinite loading screen, I haven't found why it only happens with Beforetranslation-bug-before.movAftertranslation-bug-after.movWhat I can say about this bug though is that since the code wasn't using |
|
Progress so far:
Before fixing buginfinite-loading-deeplink-before-test-desktop.movAfter fixing buginfinite-loading-deeplink-after-test-desktop.movmain branchinfinite-loading-deeplink-test-main-desktop.mov |
|
Explanation for the new NVP: When you sign in with a deep link and then log out, we still have that same initial URL. Since after logging out we're switching from I tried resetting the initial URL, the problem is that RN doesn't support it. |
|
@eVoloshchak 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] |
|
@roryabraham @fabioh8010 Would love to get your eyes on this PR to make sure we don't get any regressions :) |
|
@eVoloshchak Can we please run the tests again? Looks like an unrelated test failed. |
|
@LorenzoBloedow, I can't manually re-trigger them, but a new commit will. Could you just pull the latest main, please? That should do the trick |
|
Got some unrelated lint errors from |
This reverts commit a93c54c.
|
@eVoloshchak Can you please test the hybrid build on your iPhone? I did test the hybrid build on Android though and there's no animation but when I tested the actual production app on my physical device it also doesn't have a fade animation, so I'm guessing this is something that only happens on iOS. |
I cannot, unfortunately. The device has to be included in the team certificate, and mine isn't, as there is a limit on the number of devices I did test this by building the app locally and comparing, I don't see a difference in animation locally Testing if issues from the list in #66093 (comment) are resolved ✅
Screen.Recording.2025-08-21.at.18.00.01.movcc: @LorenzoBloedow |
|
@eVoloshchak Thanks for reviewing everything and catching that! I reintroduced the token check I'd removed here (because we couldn't reproduce the relogin bug), I must've incorrectly tried to reproduce it then, probably not using two different accounts for the repro. However, this time, instead of creating an onyx key for the last auth token, I simply used a regular variable, I think this is cleaner and less verbose, and seems to work perfectly since here we don't need the persistence of Onyx anyway. I also made sure to create a test that simulates the relogin bug: Here's the test before the fix (failing scenario): And here's the test after the fix: Also, here's a video of the non-repro after the latest fix: relogin-fix.mov |
|
Just some documentation about the tests I wrote and the issues we know of... Line 105 in fb888f2
The test above makes sure we don't try to access the previous account's report after logging out and in with a deep link, therefore, it covers:
Line 136 in fb888f2
The test above makes sure we don't reuse the last deep link auth token after signing out. It covers:
About the 2 issues above:
|
|
Tested all bugs, not able to reproduce any: Relogin bugRelogin.bug.movNot found page bug 1Not.found.page.bug.1.movNot found page bug 2Not.found.page.bug.2.movInfinite loading bug 1Infinite.loading.bug.1.movInfinite loading bug 2Infinite.loading.bug.2.movBlank screen with magic link bugBlank.screen.with.magic.link.bug.moviOS blank screen bugiOS.blank.screen.bug.mov |
|
I see tsc failed, but it appears to have been due to an unrelated issue |
|
@LorenzoBloedow, could you resolve the conflicts please? |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-08-27.at.18.55.45.movAndroid: mWeb ChromeNot found page bug 2, Infinite loading bug 2:Screen.Recording.2025-08-27.at.19.00.10.movBlank screen with magic link bug: Screen.Recording.2025-08-27.at.19.05.29.moviOS: HybridAppScreen.Recording.2025-08-27.at.19.41.49.moviOS: mWeb SafariNot found page bug 2, Infinite loading bug 2:Screen.Recording.2025-08-27.at.19.11.34.movBlank screen with magic link bug: Screen.Recording.2025-08-27.at.19.13.49.movMacOS: Chrome / SafariNot found page bug 2:Screen.Recording.2025-08-27.at.18.39.49.movInfinite loading bug 2: Screen.Recording.2025-08-27.at.18.42.22.movBlank screen with magic link bug: Screen.Recording.2025-08-27.at.18.46.27.movMacOS: DesktopRelogin bug:Screen.Recording.2025-08-27.at.18.30.07.movNot found page bug 1: Screen.Recording.2025-08-27.at.18.32.03.movNot found page bug 2: Screen.Recording.2025-08-27.at.18.34.58.movInfinite loading bug 1-2: Screen.Recording.2025-08-27.at.18.36.41.mov |
eVoloshchak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.2.0-0 🚀
|
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.2.0-5 🚀
|
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.2.0-5 🚀
|
|
Wooooooo! Deployed to prod 🚀 Great job @LorenzoBloedow @eVoloshchak |


Explanation of Change
This is a migration from
withOnyxHOC touseOnyxhook inAuthScreens.Fixed Issues
$ #65970
PROPOSAL: #65970 (comment)
MOBILE-EXPENSIFY: https://github.com/Expensify/Mobile-Expensify/pull/66093
Tests
Web/Desktop:
Relogin bug:
Not found page bug 1:
Not found page bug 2:
Infinite loading bug 1:
Infinite loading bug 2:
Blank screen with magic link bug:
iOS:
Offline tests
N/A
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))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
White screen issue:
49880-android.mov
Android: mWeb Chrome
White screen issue:
49880-android-chrome.mov
iOS: Native
White screen issue:
49880.mov
iOS: mWeb Safari
White screen issue:
49880-safari.mov
MacOS: Chrome / Safari
Blank screen with magic link:
57359.mov
MacOS: Desktop
Relogin issue:
55681.59139.mov
Infinite loading issue:
59128.59184.mov
Unrelated console errors
These also happen on
main.Reporting them here to complete the PR checklist: