feat(auth): make each phone auth step a real navigation destination - #2472
feat(auth): make each phone auth step a real navigation destination#2472demolaf wants to merge 1 commit into
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 '=='.
8dc2391 to
281692d
Compare
281692d to
23dc3e7
Compare
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.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. Four of the host tests fail against the old code, and the production call sites are mutated in both directions so reverting the wiring cannot leave the suite green.:auth:testDebugUnitTestfails on this branch and the cause is pre-existing. The suite now needs more than Gradle's default 512mTestheap: the executor dies at exit 1 after 70 of 71 suites (910 of 918 tests), killingPhoneAuthScreenVerificationLifecycleTestpartway and skippingReauthSurfaceGateTestentirely. It is not these tests — all three phone suites pass together at 512m — it is cumulative retention across the module's Robolectric suites, and the previous commit already passed with only ~26 tests of headroom.maxHeapSize = "2g"makes it green, but that masks the retention rather than fixing it, so it is deliberately left out of this PR and wants its own investigation.Maintainer note: Fixes internal CPRN-403