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
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.
e1e5941 to
bdc298d
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