Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a customizable signed-in screen slot (signedInContent) to AuthPickerView and AuthPickerContent, allowing developers to replace the default signed-in screen while retaining core authentication logic. Feedback on these changes highlights a potential SwiftUI sheet presentation conflict during account deletion, which can be resolved by utilizing the onDismiss callback. Additionally, the reviewer suggests correcting the isEmailVerified logic and documentation to prevent displaying the email verification button to users without an email address, and avoiding reporting errors when a user cancels the reauthentication flow.
| .sheet(isPresented: $showDeleteConfirmation) { | ||
| DeleteAccountConfirmationSheet( | ||
| onConfirm: { | ||
| showDeleteConfirmation = false | ||
| deleteAccount() | ||
| }, | ||
| onCancel: { | ||
| showDeleteConfirmation = false | ||
| } | ||
| ) | ||
| .presentationDetents([.medium]) | ||
| } |
There was a problem hiding this comment.
In SwiftUI, presenting a new sheet (like the reauthentication sheet) immediately after dismissing another sheet (the delete confirmation sheet) can fail because the dismissal animation of the first sheet is still in progress. To prevent this presentation conflict, we should trigger deleteAccount() only after the confirmation sheet has completely finished dismissing, using its onDismiss callback.
Note: Please also add @State private var pendingDelete = false to the properties of SignedInScreen.
.sheet(isPresented: $showDeleteConfirmation, onDismiss: {
if pendingDelete {
pendingDelete = false
deleteAccount()
}
}) {
DeleteAccountConfirmationSheet(
onConfirm: {
pendingDelete = true
showDeleteConfirmation = false
},
onCancel: {
showDeleteConfirmation = false
}
)
.presentationDetents([.medium])
}| /// Whether the user's email is verified; `true` when there is no user. | ||
| public let isEmailVerified: Bool |
There was a problem hiding this comment.
Update the documentation for isEmailVerified to reflect that it is also true when the user has no email address (e.g., phone or anonymous authentication), which helps prevent showing email verification UI to these users.
| /// Whether the user's email is verified; `true` when there is no user. | |
| public let isEmailVerified: Bool | |
| /// Whether the user's email is verified; `true` when there is no user or no email address. | |
| public let isEmailVerified: Bool |
| verifyEmail: verifyEmail, | ||
| goToMFAManagement: { authService.navigator.push(.mfaManagement) }, | ||
| user: authService.currentUser, | ||
| isEmailVerified: authService.currentUser?.isEmailVerified != false, |
There was a problem hiding this comment.
For users signed in via phone or anonymous authentication, email is nil and isEmailVerified is false. This causes the "Verify Email" button to be incorrectly displayed in the default signed-in content (and potentially in custom signed-in views). We should treat isEmailVerified as true (or not applicable) if the user does not have an email address.
| isEmailVerified: authService.currentUser?.isEmailVerified != false, | |
| isEmailVerified: authService.currentUser?.email == nil || authService.currentUser?.isEmailVerified != false, |
| if case AuthServiceError.signInCancelled = error { | ||
| // The user dismissed reauthentication; the sheet still reports it, as before. | ||
| } else { | ||
| errorMessage = authService.string.localizedErrorMessage(for: error) | ||
| } | ||
| reportError?(error) |
There was a problem hiding this comment.
When the user cancels or dismisses the reauthentication flow, AuthServiceError.signInCancelled is thrown. Currently, this error is still passed to reportError?(error), which triggers an unnecessary error alert to the user. We should avoid reporting the error when the user intentionally cancels the operation.
| if case AuthServiceError.signInCancelled = error { | |
| // The user dismissed reauthentication; the sheet still reports it, as before. | |
| } else { | |
| errorMessage = authService.string.localizedErrorMessage(for: error) | |
| } | |
| reportError?(error) | |
| if case AuthServiceError.signInCancelled = error { | |
| // The user dismissed reauthentication; do not show an error or report it. | |
| } else { | |
| errorMessage = authService.string.localizedErrorMessage(for: error) | |
| reportError?(error) | |
| } |
cc01c83 to
ca1c05e
Compare
8092009 to
79bd4e1
Compare
ca1c05e to
5336ee4
Compare
79bd4e1 to
a2ab5af
Compare
75d9824 to
ba7ecda
Compare
Add signedInContent(_:) to AuthPickerView and AuthPickerContent, so apps can replace the screen the authentication sheet shows to signed-in users. The content receives a SignedInContentState with the sign-out, delete-account, verify-email and navigation actions; an internal SignedInScreen keeps running them, including the delete confirmation, reauthentication and alerts. DefaultSignedInContent holds the stock markup and is the slot's default, and SignedInView renders it unchanged and gains a public init. The slot carries through pickerContent(_:) and pickerDestination(_:) in either order, and the progress overlay stays outside it. Swiping away a reauthentication sheet now counts as a cancel, so the operation waiting on it finishes instead of hanging.
a2ab5af to
d9f07ca
Compare
Adds
.signedInContent { state in ... }toAuthPickerView, so apps can replace the screen the authentication sheet shows to signed-in users while the library keeps signing out, deleting and reauthenticating. Based on #1412.Changes
SignedInContentState: closures for sign out, delete account, verify email and navigation, plususer,isEmailVerified,isMFAEnabled,isLoadingand a localizederrorMessage.DefaultSignedInContent(state:)holds the stock markup and is the default;SignedInViewkeeps its type, renders it unchanged and gains a public init.pickerContent(_:)andpickerDestination(_:)in either order.AuthPickerContentgains a generic parameter, but has not shipped.API Usage
Preview
Maintainer note: Fixes internal CPRN-522