Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the multi-factor authentication (MFA) enrollment UI by splitting it into a reusable state container MFAEnrolmentScreen, a default UI component DefaultMFAEnrolmentContent, and updating MFAEnrolmentView to use these new components. The review feedback suggests using SwiftUI's native @Environment(\.openURL) instead of UIApplication.shared.open for better platform support and testability. Additionally, it recommends avoiding recreating CIContext on every invocation of generateQRCode to prevent performance degradation during view updates.
| @Environment(AuthService.self) private var authService | ||
| @FocusState private var focus: FocusableField? | ||
| private let state: MFAEnrollmentContentState |
There was a problem hiding this comment.
Use SwiftUI's native openURL environment action instead of accessing UIApplication.shared directly. This is more idiomatic in SwiftUI, supports platform-agnostic URL opening, and improves testability.
| @Environment(AuthService.self) private var authService | |
| @FocusState private var focus: FocusableField? | |
| private let state: MFAEnrollmentContentState | |
| @Environment(AuthService.self) private var authService | |
| @Environment(\.openURL) private var openURL | |
| @FocusState private var focus: FocusableField? | |
| private let state: MFAEnrollmentContentState |
| Button(action: { | ||
| UIApplication.shared.open(qrURL) | ||
| }) { |
| private func generateQRCode(from string: String) -> UIImage? { | ||
| let data = Data(string.utf8) | ||
|
|
||
| guard let filter = CIFilter(name: "CIQRCodeGenerator") else { return nil } | ||
| filter.setValue(data, forKey: "inputMessage") | ||
| filter.setValue("H", forKey: "inputCorrectionLevel") | ||
|
|
||
| guard let ciImage = filter.outputImage else { return nil } | ||
|
|
||
| // Scale up the QR code for better quality | ||
| let transform = CGAffineTransform(scaleX: 10, y: 10) | ||
| let scaledImage = ciImage.transformed(by: transform) | ||
|
|
||
| let context = CIContext() | ||
| guard let cgImage = context.createCGImage(scaledImage, from: scaledImage.extent) else { | ||
| return nil | ||
| } | ||
|
|
||
| return UIImage(cgImage: cgImage) | ||
| } |
There was a problem hiding this comment.
Creating a CIContext is a very expensive operation. Since generateQRCode is called directly during the view's body rendering, recreating the CIContext on every view update will cause significant performance degradation. Reusing a single CIContext instance via a local static let avoids this overhead.
| private func generateQRCode(from string: String) -> UIImage? { | |
| let data = Data(string.utf8) | |
| guard let filter = CIFilter(name: "CIQRCodeGenerator") else { return nil } | |
| filter.setValue(data, forKey: "inputMessage") | |
| filter.setValue("H", forKey: "inputCorrectionLevel") | |
| guard let ciImage = filter.outputImage else { return nil } | |
| // Scale up the QR code for better quality | |
| let transform = CGAffineTransform(scaleX: 10, y: 10) | |
| let scaledImage = ciImage.transformed(by: transform) | |
| let context = CIContext() | |
| guard let cgImage = context.createCGImage(scaledImage, from: scaledImage.extent) else { | |
| return nil | |
| } | |
| return UIImage(cgImage: cgImage) | |
| } | |
| private func generateQRCode(from string: String) -> UIImage? { | |
| let data = Data(string.utf8) | |
| guard let filter = CIFilter(name: "CIQRCodeGenerator") else { return nil } | |
| filter.setValue(data, forKey: "inputMessage") | |
| filter.setValue("H", forKey: "inputCorrectionLevel") | |
| guard let ciImage = filter.outputImage else { return nil } | |
| // Scale up the QR code for better quality | |
| let transform = CGAffineTransform(scaleX: 10, y: 10) | |
| let scaledImage = ciImage.transformed(by: transform) | |
| static let context = CIContext() | |
| guard let cgImage = context.createCGImage(scaledImage, from: scaledImage.extent) else { | |
| return nil | |
| } | |
| return UIImage(cgImage: cgImage) | |
| } |
cc01c83 to
ca1c05e
Compare
0a6209e to
34071fe
Compare
ca1c05e to
5336ee4
Compare
34071fe to
e1e5941
Compare
75d9824 to
ba7ecda
Compare
e1e5941 to
bdc298d
Compare
9488843 to
63e4ec2
Compare
bdc298d to
72e30e2
Compare
63e4ec2 to
1a2e734
Compare
72e30e2 to
1c96100
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.
1a2e734 to
3faec1d
Compare
Add public MFAEnrolmentScreen, which owns the enrolment state, reauthentication and error reporting and passes its content an MFAEnrollmentContentState. DefaultMFAEnrolmentContent holds the stock markup, and MFAEnrolmentView now renders it through the screen.
Map finished enrolment sessions back to factor selection, require the selected factor to be allowed before starting, keep copy feedback from being cut short, clear focus between steps, and cover the step and isValid rules with unit tests.
Map sessions past expiresAt back to factor selection, only send codes from the SMS steps, disable Resend Code while loading, and note that custom content sets its own navigation title.
Move the startEnrollment, sendCode and enroll guards into Step helpers with unit tests, and replace the unused focus enum with a single phone number focus flag.
1c96100 to
055090f
Compare
3faec1d to
12b503d
Compare
Adds public
MFAEnrolmentScreen, so apps can draw the MFA enrolment screen themselves while the library keeps starting enrolment, sending codes, reauthenticating and reporting errors. Stacked on #1412.Changes
MFAEnrolmentScreenowns the enrolment state and passes its content anMFAEnrollmentContentState: the currentstep,Bindings for input, closures for actions, the TOTP secret and QR URL, plusisValid,isLoadingand a localizederrorMessage.DefaultMFAEnrolmentContentholds the stock markup, andMFAEnrolmentViewnow renders it through the screen. Reauthentication stays inside the screen.isLoadingalso covers theAuthServicesigning in.isValidrules and action guards.API Usage
Preview
Maintainer note: Fixes internal CPRN-520