Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the authentication views by introducing state-driven screen components (EmailAuthScreen, EmailLinkScreen, PasswordRecoveryScreen) to decouple UI from business logic. The review identifies several high-severity issues regarding potential race conditions when capturing email state in asynchronous tasks and inconsistencies in email validation logic. Additionally, medium-severity feedback suggests improving user experience by disabling submit buttons during loading states.
| private func sendSignInLink() { | ||
| guard isValid, !isLoading else { return } | ||
| isWorking = true | ||
| errorMessage = nil | ||
| Task { | ||
| defer { isWorking = false } | ||
| do { | ||
| try await authService.sendEmailSignInLink(email: email) | ||
| didSend = true | ||
| authService.emailLink = email | ||
| } catch { | ||
| errorMessage = authService.string.localizedErrorMessage(for: error) | ||
| reportError?(error) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Capturing email dynamically inside the asynchronous Task can lead to race conditions if the user edits the text field while the request is in flight. If the email is modified before the request completes, the wrong email address will be saved to authService.emailLink, which will break the email link sign-in flow when the user opens the link. Capture the email address locally before starting the Task to ensure consistency.
private func sendSignInLink() {
guard isValid, !isLoading else { return }
let emailToSend = email
isWorking = true
errorMessage = nil
Task {
defer { isWorking = false }
do {
try await authService.sendEmailSignInLink(email: emailToSend)
didSend = true
authService.emailLink = emailToSend
} catch {
errorMessage = authService.string.localizedErrorMessage(for: error)
reportError?(error)
}
}
}| private var isValid: Bool { | ||
| CommonUtils.isValidEmail(email) | ||
| } |
There was a problem hiding this comment.
There is an inconsistency in email validation. The screen uses CommonUtils.isValidEmail(email) to enable/disable the submit button, while the text field in DefaultEmailLinkContent uses FormValidators.email for inline validation. This can lead to cases where the button is enabled but the field shows an error, or the button is disabled with no error shown. Use FormValidators.email.isValid(input: email) to ensure consistent validation across the UI. Note that you will also need to add import FirebaseAuthUIComponents at the top of this file.
| private var isValid: Bool { | |
| CommonUtils.isValidEmail(email) | |
| } | |
| private var isValid: Bool { | |
| FormValidators.email.isValid(input: email) | |
| } |
| private var isValid: Bool { | ||
| CommonUtils.isValidEmail(email) | ||
| } |
There was a problem hiding this comment.
There is an inconsistency in email validation. The screen uses CommonUtils.isValidEmail(email) to enable/disable the submit button, while the text field in DefaultPasswordRecoveryContent uses FormValidators.email for inline validation. This can lead to cases where the button is enabled but the field shows an error, or the button is disabled with no error shown. Use FormValidators.email.isValid(input: email) to ensure consistent validation across the UI. Note that you will also need to add import FirebaseAuthUIComponents at the top of this file.
| private var isValid: Bool { | |
| CommonUtils.isValidEmail(email) | |
| } | |
| private var isValid: Bool { | |
| FormValidators.email.isValid(input: email) | |
| } |
| private func sendResetLink() { | ||
| guard isValid, !isWorking else { return } | ||
| isWorking = true | ||
| errorMessage = nil | ||
| Task { | ||
| defer { isWorking = false } | ||
| do { | ||
| try await authService.sendPasswordRecoveryEmail(email: email) | ||
| sentEmail = email | ||
| didSend = true | ||
| } catch { | ||
| errorMessage = authService.string.localizedErrorMessage(for: error) | ||
| reportError?(error) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Capturing email dynamically inside the asynchronous Task can lead to race conditions if the user edits the text field while the request is in flight. If the email is modified before the request completes, the wrong email address will be saved to sentEmail, displaying the incorrect email in the success sheet. Capture the email address locally before starting the Task to ensure consistency.
| private func sendResetLink() { | |
| guard isValid, !isWorking else { return } | |
| isWorking = true | |
| errorMessage = nil | |
| Task { | |
| defer { isWorking = false } | |
| do { | |
| try await authService.sendPasswordRecoveryEmail(email: email) | |
| sentEmail = email | |
| didSend = true | |
| } catch { | |
| errorMessage = authService.string.localizedErrorMessage(for: error) | |
| reportError?(error) | |
| } | |
| } | |
| } | |
| private func sendResetLink() { | |
| guard isValid, !isWorking else { return } | |
| let emailToSend = email | |
| isWorking = true | |
| errorMessage = nil | |
| Task { | |
| defer { isWorking = false } | |
| do { | |
| try await authService.sendPasswordRecoveryEmail(email: emailToSend) | |
| sentEmail = emailToSend | |
| didSend = true | |
| } catch { | |
| errorMessage = authService.string.localizedErrorMessage(for: error) | |
| reportError?(error) | |
| } | |
| } | |
| } |
| .frame(maxWidth: .infinity) | ||
| } | ||
| } | ||
| .disabled(!state.isValid) |
There was a problem hiding this comment.
The submit button is not disabled when state.isLoading is true. Although the screen's submit logic guards against multiple submissions, disabling the button during loading provides a better UX and prevents redundant tap interactions.
| .disabled(!state.isValid) | |
| .disabled(!state.isValid || state.isLoading) |
| .frame(maxWidth: .infinity) | ||
| } | ||
| .authCTAButtonStyle() | ||
| .disabled(!state.isValid) |
| .padding(.vertical, 8) | ||
| .frame(maxWidth: .infinity) | ||
| } | ||
| .disabled(!state.isValid) |
cc01c83 to
ca1c05e
Compare
eb199e8 to
d5a1da2
Compare
5336ee4 to
75d9824
Compare
d5a1da2 to
0720f8c
Compare
75d9824 to
ba7ecda
Compare
0720f8c to
cf0ffbe
Compare
9488843 to
63e4ec2
Compare
ca64f9f to
3145692
Compare
63e4ec2 to
1a2e734
Compare
3145692 to
e2ef03d
Compare
…creens Add EnterPhoneNumberScreen and EnterVerificationCodeScreen, generic over content that receives a PhoneNumberContentState or VerificationCodeContentState. The screens own the input state and the send, verify, resend and change-number actions; the content only draws. DefaultEnterPhoneNumberContent and DefaultEnterVerificationCodeContent hold the stock markup, and the existing views render them through the new screens. The verification code screen adds a resend button with a 60 second countdown. Verifying now goes through signIn(credentials:), so an MFA challenge reaches the resolution screen instead of being dropped. isLoading also covers sending the code, which previously showed no progress and allowed repeated taps.
Set the in-flight flag before starting each action's Task, so a quick second tap can't send two codes or overlap verify and resend. Only clear the navigator on .signedIn, so an MFA challenge with no handler doesn't drop the user back to the picker. Type the state closures as @mainactor and give both state structs public initializers, matching AuthMethodPickerState. Clear errorMessage when the input changes, document every field, disable Change number while loading, and show the resend countdown as its own text instead of a concatenated string.
Add EmailAuthScreen, PasswordRecoveryScreen and EmailLinkScreen, which own the email flows' state and logic and hand their content a state struct. The stock markup moves to DefaultEmailAuthContent, DefaultPasswordRecoveryContent and DefaultEmailLinkContent, and the existing views render them through the screens.
Add goToEmailLink and hideSuccess to the state structs, keep the recovery screen when its success sheet is swiped away, handle an opened email link once even when custom content has several root views, move focus to confirm password on Go during sign-up, and align the recovery screen's isLoading with the other screens.
…al and legacy recovery
1a2e734 to
3faec1d
Compare
e2ef03d to
d5cfb5c
Compare
3faec1d to
12b503d
Compare
Adds public
EmailAuthScreen,PasswordRecoveryScreenandEmailLinkScreen, so apps can draw the email sign-in screens themselves while the library keeps signing in, sending links, navigating and reporting errors. Builds on #1412.AuthViewgainssignInandsignUpcases, so an exhaustiveswitchoverAuthViewneeds a new branch or adefault:.Changes
EmailAuthContentState,PasswordRecoveryContentStateorEmailLinkContentState:Bindings for input, closures for actions, plusisValid,isLoadingand a localizederrorMessage.DefaultEmailAuthContent,DefaultPasswordRecoveryContentandDefaultEmailLinkContenthold the stock markup, andEmailAuthView,PasswordRecoveryViewandEmailLinkViewnow render them through the screens.signInandsignUproute MFA and account conflicts to their handlers like the phone screens, and every action ignores taps while invalid or loading.AuthView.signInandAuthView.signUproutes, shown by default asSignInViewandSignUpView, reached withAuthMethodPickerState.goToEmailSignInandEmailAuthContentState.goToSignUp. They close once signed in, andEmailAuthScreen(flow:)fixes a screen's flow; the stock first screen still toggles in place.API Usage
Preview
Maintainer note: Fixes internal CPRN-519