feat(auth): make each phone auth step a real navigation destination - #2472
Conversation
There was a problem hiding this comment.
Code Review
This pull request migrates the Firebase Auth UI navigation from Navigation 2 to Navigation 3, introducing a serialized AuthRoute back-stack structure and managing destinations through NavDisplay and rememberNavBackStack. The review feedback identifies three key areas for improvement: first, FlowEntry and its implementing objects must be annotated with @serializable to prevent compilation errors under the serializable AuthRoute hierarchy; second, PhoneAuthScreen in ReauthDestinations.kt is missing hosted navigation parameters, causing it to run in un-hosted mode during reauthentication; and third, using the 'is' operator with the data object AuthRoute.MfaChallenge in FirebaseAuthScreen.kt produces a compiler warning and should be replaced with the equality operator '=='.
23dc3e7 to
91a68db
Compare
91a68db to
08e8887
Compare
* fix(auth)!: remove non-functional MFA recovery codes (#2457) * refactor(auth): scope phone verification loading state to the composition that owns it (#2454) * refactor(auth): let the phone screen own its verification loading state * fix(auth): read auth state per authUI instance when clearing loading on dispose * fix(e2e): fix flaky Google credential-linking test (#2455) * fix(auth): complete MFA challenge sign-in navigation and success callback (#2458) * feat(auth): add stable resource ids for auth input fields (#2449) * fix(auth): log diagnostic warning when Google sign-in NoCredentialException fallback exhausts (cherry picked from commit 6fa4f00) (cherry picked from commit aca99e7) (cherry picked from commit 8784b13ce29739ed7785aab9ab6fe4faad2a2981) * fix(e2e): retry and fail loudly when clearing Firebase Auth emulator data (cherry picked from commit 8eca4fa) (cherry picked from commit ac61312) (cherry picked from commit 126ba985660ff272dbfda427b644ad4fb1ca6197) * fix(e2e): don't swallow InterruptedException in emulator clear retry loop (cherry picked from commit a87cfe3) (cherry picked from commit 3f4ae70) (cherry picked from commit 1f1ada380039ed5e7476dfc2e38a683e6edb4549) * feat(auth): reshape reauthContent into a ReauthContentState content slot (#2452) * feat(auth): reshape reauthContent into a ReauthContentState content slot * fix(auth): address reauth review findings and retain state across recreation * refactor(auth): make reauthentication a request-scoped state machine * fix(auth): keep a proved reauthentication alive when its operation signs out * fix(auth): tear down phone verification when a reauthentication attempt fails * test(auth): cover sign-out-during-retry and phone reauth failure end to end * test(auth): drop the flaky phone reauth e2e case * test(auth): pin onComplete and factor refresh on successful MFA enrollment (#2462) * test(auth): pin onComplete and factor refresh on successful MFA enrollment * test(auth): pin the SMS MFA enrollment route through send, verify and resend * test(auth): pin the missing TOTP secret guard and the resend's clean enroll * feat(auth)!: make each email sign-in mode a real navigation destination (#2466) * feat(auth): make each MFA enrollment step a real navigation destination (#2467) * feat(auth)!: migrate auth navigation to Navigation 3 (#2469) * feat(auth): make reauthentication a navigation destination (#2471) * fix(auth): clear MFA enrollment flow state on every entry (#2473) * feat(auth): make each phone auth step a real navigation destination (#2472) * fix(auth): don't strand phone-only users on email verification after reload (#2474) * test(auth): cover the write-through recoveries and the custom reauth slot's sheet chrome (#2476) * test(auth): cover the three recoveries that write a value through to the step they move to * test(auth): assert a custom reauth slot's email and phone steps stay inside the library sheet --------- Co-authored-by: Ademola Fadumo <[email protected]> Co-authored-by: demolaf <[email protected]>
The phone flow declared two
AuthRoute.Phonesteps and registered both as destinations, but both mapped to the same entry body and nothing ever navigated toEnterVerificationCode—PhoneAuthScreenkept the step in its ownrememberSaveable. So code entry got no back-stack entry of its own: system back left the whole flow instead of returning to number entry, and the configured transitions never animated between the two steps.Each step is now its own destination rendering its own step, and the state that has to outlive a step switch moves into a
PhoneAuthFlowStateholder remembered above theNavDisplay.PhoneAuthScreengains nullablestep/onNavigateToStep/onNavigateBack/flowStateparameters, all-or-none likeMfaEnrollmentScreen, so calling it directly behaves exactly as before.PhoneAuthDestinations.kt: the two entries,PhoneAuthFlowState,navigateToPhoneStepandexitPhoneAuth, which truncates to the flow's lowest step so leaving drops every entry it pushed rather than one.PhoneAuthScreen.kt: the verification collection now runs on a scope that outlives the step, so pushing code entry no longer cancels SMS auto-retrieval mid-flight.ReauthDestinations.kt: the reauth phone branch passes the hosted parameters too, matchingEmailAuthStep— reauthentication already hosted email per-step but rendered number entry for a key naming code entry. Reauth gets its ownPhoneAuthFlowStatekeyed onrequestId, not the main flow's.FirebaseAuthScreen.kt: anIdleretraction reached on number entry stays there. "Change number" retracts throughIdle, which previously reset a multi-provider config out to the method picker.Added
PhoneAuthRouteNavigationTestandPhoneAuthHostDestinationsTest. Several tests fail against the old code — including back from code entry dismissing the reauth sheet instead of returning to number entry — and the production call sites are mutated in both directions so reverting the wiring cannot leave the suite green.auth/build.gradle.ktsraises theTestheap to 2g. The module's suite had outgrown Gradle's 512m default and was dying withjava.lang.OutOfMemoryErroron the test worker part-way through — CI showed it on both theTest workerandFinalizerthreads, and locally the JVM was too starved to print the error at all, taking the run down after 70 of 71 suites. It is cumulative retention across the module's Robolectric suites rather than anything in these tests — all the phone suites pass together at 512m, and the previous commit passed with only ~26 tests of headroom — so this is a workaround and the retention is worth its own look.Maintainer note: Fixes internal CPRN-403