Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the authentication UI flow in FirebaseAuthSwiftUI by extracting the sheet presentation, navigation stack, and error/MFA handling into a new AuthPickerContent view. This allows AuthPickerView to support customizable first screens and destination views via pickerContent and pickerDestination modifiers, while falling back to the newly introduced DefaultAuthPickerContent and DefaultAuthPickerDestination. Key feedback points out that DefaultAuthPickerContent's generic parameter is overly restricted by its initializer, a UIKit dependency is introduced via UIColor.label, and a redundant Task dispatch is used in reportError on a MainActor-annotated struct.
| public struct DefaultAuthPickerContent<AuthMethodPicker: View>: View { | ||
| public init() where AuthMethodPicker == DefaultAuthMethodPicker { | ||
| authMethodPicker = { DefaultAuthMethodPicker() } | ||
| } |
There was a problem hiding this comment.
The DefaultAuthPickerContent struct is generic over AuthMethodPicker, but the only initializer provided restricts AuthMethodPicker to DefaultAuthMethodPicker. This prevents callers from ever providing a custom AuthMethodPicker view, which defeats the purpose of the generic parameter.
To allow callers to customize the provider buttons list while keeping the default logo, email sign-in, and terms, please add a public initializer that accepts a custom @ViewBuilder closure for AuthMethodPicker.
| public struct DefaultAuthPickerContent<AuthMethodPicker: View>: View { | |
| public init() where AuthMethodPicker == DefaultAuthMethodPicker { | |
| authMethodPicker = { DefaultAuthMethodPicker() } | |
| } | |
| public struct DefaultAuthPickerContent<AuthMethodPicker: View>: View { | |
| public init(@ViewBuilder authMethodPicker: @escaping () -> AuthMethodPicker) { | |
| self.authMethodPicker = authMethodPicker | |
| } | |
| public init() where AuthMethodPicker == DefaultAuthMethodPicker { | |
| self.authMethodPicker = { DefaultAuthMethodPicker() } | |
| } |
| Image(systemName: "xmark") | ||
| .foregroundStyle(Color(UIColor.label)) | ||
| } |
There was a problem hiding this comment.
Using Color(UIColor.label) introduces a dependency on UIKit (via UIColor), which can cause compilation issues on non-iOS platforms (like macOS) and relies on transitive imports since UIKit is not explicitly imported in this file.
Since this is a SwiftUI view, you can use the platform-agnostic Color.primary instead, which automatically adapts to light and dark modes.
| Image(systemName: "xmark") | |
| .foregroundStyle(Color(UIColor.label)) | |
| } | |
| Image(systemName: "xmark") | |
| .foregroundStyle(Color.primary) | |
| } |
| private func reportError(_ error: Error) { | ||
| Task { @MainActor in | ||
| self.error = AlertError( | ||
| message: authService.string.localizedErrorMessage(for: error), | ||
| underlyingError: error | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Since AuthPickerContent is annotated with @MainActor, the reportError method is already guaranteed to run on the main actor. Spawning a new asynchronous Task to update self.error is redundant and introduces an unnecessary dispatch hop. You can update the state directly.
private func reportError(_ error: Error) {
self.error = AlertError(
message: authService.string.localizedErrorMessage(for: error),
underlyingError: error
)
}
Adds
.pickerContentand.pickerDestinationtoAuthPickerView, so apps can replace the auth sheet's first screen or any pushed screen while the library keeps driving navigation, MFA and account conflicts. Stacked on #1402.Changes
AuthPickerViewkeeps its signature. The new modifiers returnAuthPickerContent, which now owns the sheet,NavigationStack, error alert, and MFA and account-conflict handling.DefaultAuthPickerContent,DefaultAuthMethodPickerandDefaultAuthPickerDestination, holding the markup that was private insideAuthPickerView..pickerContentreplaces only the signed-out screen.SignedInViewand the "Authenticating..." overlay stay inAuthPickerContent, so custom content can't hide them.DefaultAuthPickerContentis generic over its provider list from the start, so the custom-layout initializer in the next PR is additive.Naming, pending team review
AuthPickerContent(the wrapper) sits besideDefaultAuthPickerContent(the default for itsPickerContentslot). Kept for now; the alternative is renaming the wrapper, e.g.ComposedAuthPickerView, before release.API Usage
Preview
Maintainer note: Fixes internal CPRN-516