Feature/better error handling

[JNdhlovu]

Story: https://app.shortcut.com/smileid/story/13730/improve-error-handling-on-android-and-ios-sdk
Improve Error Handling on android and ios sdk.

Summary

Better abstraction for

1. Smartselfie enrollment
2. Smartselfie authentication
3. Document verification
4. Enhanced document verification
5. Biometric KYC

Known Issues

n/a

Test Instructions

1. Test the jobs from the orchestrated screens paths
2. Test offline mode
3. Capture jobs and then use the submitJob method to make sure everything is working fine

Screenshot

n/a

[prfectionist]

PR Reviewer Guide 🔍

(Review updated until commit d22c8c7)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Error Handling The error handling flow in proxyErrorHandler() may lead to inconsistent error states. The method attempts to handle offline failures but could end up showing both offline success and error states. Resource Management The executeUpload() method creates and holds all files in memory before zipping. For large files this could lead to memory issues. Consider streaming the zip creation. Code Duplication The createSuccessResult() method duplicates authentication and API response logic from executeApiSubmission(). Consider refactoring to avoid duplicate code.

[github-actions]

	Warnings
⚠️	The source files were changed, but the tests remain unmodified. Consider updating or adding to the tests to match the source changes.

Generated by 🚫 Danger Swift against 4ed768b

[prfectionist]

PR Code Suggestions ✨

Latest suggestions up to d22c8c7
Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Improve error handling by adding explicit checks and error messages for missing or invalid image data Add error handling for the case where selfieImage or livenessImages are nil or empty before accessing them in the onFinished method. Sources/SmileID/Classes/SelfieCapture/SelfieViewModel.swift [334-339] -if let selfieImage = selfieImage, - let selfiePath = getRelativePath(from: selfieImage), - livenessImages.count == numLivenessImages, - !livenessImages.contains(where: { getRelativePath(from: $0) == nil }) -{ +guard let selfieImage = selfieImage else { + callback.didError(error: SmileIDError.unknown("Missing selfie image")) + return +} +guard let selfiePath = getRelativePath(from: selfieImage), + livenessImages.count == numLivenessImages, + !livenessImages.contains(where: { getRelativePath(from: $0) == nil }) else { + callback.didError(error: SmileIDError.unknown("Invalid image paths")) + return +} Suggestion importance[1-10]: 8 Why: The suggestion significantly improves error handling by adding explicit guard statements and specific error messages, making the code more robust and easier to debug.	8
	Add proper error handling when required files are missing instead of silently failing Add error handling for the case where selfieFile or livenessFiles are nil in onFinished(). Currently the function silently fails if these are nil. Sources/SmileID/Classes/BiometricKYC/OrchestratedBiometricKycViewModel.swift [57-64] func onFinished(delegate: BiometricKycResultDelegate) { if let selfieFile = selfieFile, let livenessFiles = livenessFiles, let selfiePath = getRelativePath(from: selfieFile) { delegate.didSucceed( selfieImage: selfiePath, livenessImages: livenessFiles.compactMap { getRelativePath(from: $0) }, + } else { + delegate.didFail(error: SmileIDError.unknown("Missing required selfie or liveness files")) + } Suggestion importance[1-10]: 8 Why: The suggestion addresses a critical issue where missing required files could lead to silent failures. Adding explicit error handling improves reliability and debugging.	8
Best practice	Align class access level with its public interface to ensure proper accessibility Make the class public since it contains a public initializer. Having a public initializer in an internal class can lead to accessibility issues. Sources/SmileID/Classes/JobSubmission/DocumentVerificationSubmission.swift [8] -class DocumentVerificationSubmission: BaseDocumentVerificationSubmission<DocumentVerificationResult> { +public class DocumentVerificationSubmission: BaseDocumentVerificationSubmission<DocumentVerificationResult> { Suggestion importance[1-10]: 8 Why: Having a public initializer in an internal class creates an accessibility mismatch that could cause compilation issues. Making the class public is essential for proper API design and usage.	8
	Add a safeguard against potential memory issues by limiting the number of stored images Add a check for the maximum size of livenessImages array to prevent potential memory issues with too many images. Sources/SmileID/Classes/SelfieCapture/SelfieViewModel.swift [41] -var livenessImages: [URL] = [] +private let maxLivenessImages = 20 // Or appropriate limit +var livenessImages: [URL] = [] { + didSet { + if livenessImages.count > maxLivenessImages { + livenessImages.removeFirst(livenessImages.count - maxLivenessImages) + } + } +} Suggestion importance[1-10]: 7 Why: The suggestion adds an important safety measure to prevent potential memory issues by limiting the size of the livenessImages array, which could be critical in a production environment.	7
	Prevent potential memory leaks by properly managing task lifecycle Add error handling for potential memory leaks by canceling any ongoing Task when the view model is deallocated. Sources/SmileID/Classes/JobSubmission/Base/BaseSubmissionViewModel.swift [89-95] +private var submissionTask: Task<Void, Never>? + internal func submitJob( jobId: String, skipApiSubmission: Bool = false, offlineMode: Bool = SmileID.allowOfflineMode ) { triggerProcessingState() - - Task { + submissionTask?.cancel() + submissionTask = Task { Suggestion importance[1-10]: 7 Why: The suggestion helps prevent memory leaks by properly managing async task lifecycle, which is important for app stability and resource management.	7
Enhancement	Implement retry mechanism with exponential backoff for network operations to improve reliability Add retry logic with exponential backoff for network operations in executeUpload() to handle transient network failures. Sources/SmileID/Classes/JobSubmission/Base/BaseJobSubmission.swift [134-158] private func executeUpload( authResponse: AuthenticationResponse?, - prepUploadResponse: PrepUploadResponse + prepUploadResponse: PrepUploadResponse, + retryCount: Int = 3, + baseDelay: TimeInterval = 1.0 ) async throws { do { let uploadRequest = createUploadRequest(authResponse: authResponse) let allFiles: [URL] + for attempt in 0..<retryCount { + do { + try await attemptUpload(uploadRequest, prepUploadResponse) + return + } catch { + if attempt == retryCount - 1 { throw error } + try await Task.sleep(nanoseconds: UInt64(baseDelay * pow(2.0, Double(attempt)) * 1_000_000_000)) + } + } Suggestion importance[1-10]: 8 Why: Adding retry logic with exponential backoff is crucial for handling transient network failures and improving the reliability of network operations.	8
Maintainability	Maintain consistent indentation throughout the codebase Fix the indentation of the init method parameters to match the standard Swift style guide (4 spaces). Sources/SmileID/Classes/JobSubmission/DocumentVerificationSubmission.swift [10-13] public init( - jobId: String, - userId: String, - countryCode: String, + jobId: String, + userId: String, + countryCode: String, Suggestion importance[1-10]: 4 Why: While the indentation change improves code readability and follows Swift style guidelines, it's a relatively minor formatting issue that doesn't affect functionality.	4
	Maintain consistent spacing between class members Remove extra blank line between init and createResultInstance methods to maintain consistent spacing. Sources/SmileID/Classes/JobSubmission/DocumentVerificationSubmission.swift [37-40] ) } - override func createResultInstance( Suggestion importance[1-10]: 3 Why: The suggestion addresses a minor code style issue by removing an extra blank line. While it improves code consistency, it has minimal impact on code quality or functionality.	3

---

Previous suggestions

Suggestions up to commit 046089a

Category	Suggestion	Score
Possible bug	Fix incomplete enum case assignment by providing the required associated value Fix the incomplete processing state assignment in handleOfflineSuccess() by providing the required ProcessingState value. Sources/SmileID/Classes/BiometricKYC/OrchestratedBiometricKycViewModel.swift [302] -self.step = .processing(<#T##ProcessingState#>) +self.step = .processing(.success) Suggestion importance[1-10]: 9 Why: The suggestion fixes a critical compilation error by providing the required ProcessingState value for the enum case, which would prevent the code from building.	9
Possible issue	Validate the exact number of required liveness images instead of just checking for non-emptiness The validation of smartSelfieLivenessImages in getApiResponse uses isEmpty which is less precise than checking for the expected count. Consider validating against the required number of liveness images. Sources/SmileID/Classes/JobSubmission/SelfieSubmission.swift [99-103] guard let smartSelfieImage = smartSelfieImage, - !smartSelfieLivenessImages.isEmpty + smartSelfieLivenessImages.count == livenessFiles.count else { - throw SmileIDError.unknown("Selfie submission failed") + throw SmileIDError.unknown("Selfie submission failed: Missing required images") } Suggestion importance[1-10]: 7 Why: This is a meaningful improvement that adds more precise validation, ensuring the exact number of required liveness images are present rather than just checking for non-emptiness, which could prevent potential issues.	7
Best practice	Maintain consistent asynchronous execution flow by using async/await for storage operations Consider using async/await for LocalStorage operations in handleOfflinePreparation() to maintain consistent asynchronous flow. Sources/SmileID/Classes/JobSubmission/Base/BaseJobSubmission.swift [73-80] public func handleOfflinePreparation() async throws { let authRequest = createAuthRequest() - try LocalStorage.createAuthenticationRequestFile(jobId: jobId, authentationRequest: authRequest) - try LocalStorage.createPrepUploadFile( + try await LocalStorage.createAuthenticationRequestFile(jobId: jobId, authentationRequest: authRequest) + try await LocalStorage.createPrepUploadFile( jobId: jobId, prepUpload: createPrepUploadRequest() ) } Suggestion importance[1-10]: 7 Why: The suggestion enhances code consistency and prevents potential concurrency issues by properly marking storage operations as asynchronous using async/await.	7
Maintainability	Improve code readability and maintainability by extracting nested logic into separate methods Avoid nested do-catch blocks in executeUpload() by extracting file handling logic into a separate method. Sources/SmileID/Classes/JobSubmission/Base/BaseJobSubmission.swift [140-152] +private func getAllFiles() throws -> [URL] { + let livenessFiles = try LocalStorage.getFilesByType(jobId: jobId, fileType: .liveness) ?? [] + let additionalFiles = try [ + LocalStorage.getFileByType(jobId: jobId, fileType: .selfie), + LocalStorage.getFileByType(jobId: jobId, fileType: .documentFront), + LocalStorage.getFileByType(jobId: jobId, fileType: .documentBack), + LocalStorage.getInfoJsonFile(jobId: jobId) + ].compactMap { $0 } + return livenessFiles + additionalFiles +} + do { let uploadRequest = createUploadRequest(authResponse: authResponse) - let allFiles: [URL] - do { - let livenessFiles = try LocalStorage.getFilesByType(jobId: jobId, fileType: .liveness) ?? [] - let additionalFiles = try [ - LocalStorage.getFileByType(jobId: jobId, fileType: .selfie), - LocalStorage.getFileByType(jobId: jobId, fileType: .documentFront), - LocalStorage.getFileByType(jobId: jobId, fileType: .documentBack), - LocalStorage.getInfoJsonFile(jobId: jobId) - ].compactMap { $0 } - allFiles = livenessFiles + additionalFiles - } catch { - throw error - } + let allFiles = try getAllFiles() Suggestion importance[1-10]: 6 Why: The suggestion improves code organization and readability by extracting nested file handling logic into a dedicated method, making the code more maintainable and easier to test.	6
Enhancement	Combine redundant array emptiness checks into a single guard statement The guard statement checking for empty results array is redundant since you already check results.count == 0 right after. Combine these checks for better code clarity. Sources/SmileID/Classes/SelfieCapture/SelfieViewModel.swift [125-132] -guard let results = request.results as? [VNFaceObservation] else { - print("Did not receive the expected [VNFaceObservation]") +guard let results = request.results as? [VNFaceObservation], !results.isEmpty else { + DispatchQueue.main.async { self.directive = "Instructions.UnableToDetectFace" } return } -if results.count == 0 { - DispatchQueue.main.async { self.directive = "Instructions.UnableToDetectFace" } - Suggestion importance[1-10]: 5 Why: The suggestion offers a valid code improvement by combining two separate checks into a single guard statement, making the code more concise and efficient while maintaining the same functionality.	5

[github-actions]

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[github-actions]

This PR was closed because it has been stalled for 7 days with no activity.

[prfectionist]

Persistent review updated to latest commit d22c8c7

[robin-ankele]

@JNdhlovu please fix the linter issues. Otherwise code-wise it looks fine to me. I've tested:

• SmartSelfie Enrollment
• SmartSelfie Authentication
• Document Verification - I always get an unknown error at the end of the flow even though the verification passes - https://portal.usesmileid.com/admin/002/job_results/production/0000177020

I couldn't test the following, since I don't have the relevant document types:

• Enhanced Document Verification
• Enhanced KYC
• Biometric KYC

[robin-ankele]

@JNdhlovu you'll have to remove all those tab whitespaces otherwise the linter will complain.

[github-actions]

• ⚠️ Opening braces should be preceded by a single space and on the same line as the declaration. (opening_brace)

[github-actions]

• ⚠️ Colons should be next to the identifier when specifying a type and next to the key in dictionary literals. (colon)

[github-actions]

• ⚠️ Collection literals should not have trailing commas. (trailing_comma)

[github-actions]

• ⚠️ Line should be 120 characters or less: currently 127 characters (line_length)

[github-actions]

• ⚠️ Line should be 120 characters or less: currently 125 characters (line_length)

[github-actions]

• ⚠️ Line should be 120 characters or less: currently 125 characters (line_length)

[github-actions]

• ⚠️ Lines should not have trailing whitespace. (trailing_whitespace)

[github-actions]

• ⚠️ Lines should not have trailing whitespace. (trailing_whitespace)

[github-actions]

• ⚠️ Line should be 120 characters or less: currently 156 characters (line_length)

[github-actions]

• ⚠️ TODOs should be resolved (Use mouth deformation as an al...). (todo)