Migration from platform views 🚧 #136

CoderNamedHendrick · 2025-01-05T17:37:41Z

Story: #135

Summary

An attempt to migrate from embedding platform views and instead call the native navigation APIs. This update removes the need to use callbacks that return JSON strings in embedded platform views and simply sends type-safe responses to the Flutter application.

Known Issues

There are no issues on Android.
On iOS, this change requires a new setup step, where the user needs to wrap the FlutterViewController with a UINavigationController in the app delegate. This is necessary to have page-to-page-like (the alternative is modal-like in the absence of the controller) navigation. Any ideas on how to work around this would be appreciated. This issue on iOS has been resolved see here

Test Instructions

Run the sample app on both iOS and Android and test the selfie enrollment(the only flow currently migrated and awaiting feedback on implementation)

Screenshot

If applicable (e.g. UI changes), add screenshots to help explain your work.
iOS and Android previews showing migrated smartSelfieEnrollment flows.

prfectionist · 2025-01-05T17:38:35Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 135 - Partially compliant Fully compliant requirements: Migrated selfie enrollment flow from platform views to native navigation Not compliant requirements: Other flows still need to be migrated
⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Code Duplication Duplicated error handling logic across different button handlers. Consider extracting common error handling into a shared function. Memory Management Potential memory leak in SmartSelfieDelegateController due to strong reference cycle between controller and result closure Navigation Setup Navigation controller setup may cause issues if window or root view controller are nil. Add proper nil checks and error handling.

CoderNamedHendrick · 2025-01-05T17:38:47Z

@jumaallan I'll be awaiting your feedback

prfectionist · 2025-01-05T17:39:10Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Best practice	Clear completion handler after use to prevent memory leaks Add memory management by setting the completion handler to nil after use to prevent retain cycles and memory leaks. ios/Classes/SmileIDSmartSelfieEnrollmentView.swift [60-61] completion?(.success(result)) uiViewController?.popViewController(animated: true) +completion = nil Suggestion importance[1-10]: 8 Why: The suggestion addresses a potential memory leak by properly cleaning up the completion handler after use, which is crucial for memory management in iOS.	8
Possible bug	Add null safety checks when accessing optional properties to prevent runtime exceptions Add error handling around the print statements to avoid potential null pointer exceptions when accessing result properties. example/lib/main.dart [208-211] -print('enrollment selfie: ${result.selfieFile}'); -print('enrollment liveness: ${result.livenessFiles}'); -print('enrollment apiResponse: ${result.apiResponse}'); -print('enrollment didSubmitBiometricKycJob: ${result.didSubmitBiometricKycJob}'); +print('enrollment selfie: ${result.selfieFile ?? 'null'}'); +print('enrollment liveness: ${result.livenessFiles ?? []}'); +print('enrollment apiResponse: ${result.apiResponse ?? {}}'); +print('enrollment didSubmitBiometricKycJob: ${result.didSubmitBiometricKycJob ?? false}'); Suggestion importance[1-10]: 7 Why: The suggestion adds important null safety checks to prevent potential runtime exceptions when accessing optional properties, which could crash the app if any of these values are null.	7
Possible issue	Add additional null safety checks when accessing intent extras Add null checks when accessing bundle extras to prevent potential NullPointerException. android/src/main/kotlin/com/smileidentity/flutter/SmileIDSmartSelfieEnrollmentActivity.kt [20-22] -val userId = intent.getStringExtra("userId") ?: randomUserId() +val userId = intent.getStringExtra("userId").takeIf { !it.isNullOrEmpty() } ?: randomUserId() val allowNewEnroll = intent.getBooleanExtra("allowNewEnroll", false) val allowAgentMode = intent.getBooleanExtra("allowAgentMode", false) Suggestion importance[1-10]: 4 Why: While the existing code already has basic null handling with the Elvis operator, the suggestion adds an extra layer of validation for empty strings, but the improvement is minor since the current implementation is already safe.	4

android/src/main/res/values/strings.xml

example/ios/Podfile.lock

ios/Classes/SmileIDSmartSelfieEnrollmentView.swift

pigeon/messages.dart

android/build.gradle

android/src/main/AndroidManifest.xml

example/lib/main.dart

jumaallan · 2025-01-08T18:15:54Z

I also noticed since we now use an activity, we now have a topbar added - not sure if we want to keep that or add a theme to hide it

see these two screenshots

I think attribution does not work too, we might need to see why that is broken (maybe the screen content gets pushed way down)

CoderNamedHendrick · 2025-01-08T18:36:14Z

I also noticed since we now use an activity, we now have a topbar added - not sure if we want to keep that or add a theme to hide it

see these two screenshots

I think attribution does not work too, we might need to see why that is broken (maybe the screen content gets pushed way down)

I just tested this and the show attribution is working on my device, you are right about the screen content being pushed down. Hiding the top bar should resolve this right? The top bar being there or not doesn't make much of a difference, most users of the current platform views simply follow the smileID docs and place a topbar with smile ID as the title :)

jumaallan · 2025-01-08T18:37:02Z

d resolve this right? The top bar bei

Yeah, go ahead and remove it then - I suspected the attribution was being pushed down the screen

CoderNamedHendrick · 2025-01-08T18:42:10Z

I'll update the PR before the end of today to get your feedback, @jumaallan. Thanks for the quick responses! 😊

jumaallan · 2025-01-08T18:46:48Z

e PR before the end of today to g

While you are at it - can you add a dart linter and maybe add it to github actions too?

we don't lint dart and we would love to add that too

- add format/lint to build script

…sult - added deprecation notice for SmileIDSmartSelfieEnrollment widget

CoderNamedHendrick · 2025-01-08T20:59:21Z

@jumaallan I've attended to the issues you raised. Awaiting your feedback

JNdhlovu

Excellent work, left feedback also maybe add TODOs for which other items will be changed to reflect this change

iOS however we need to find a way to make this work without asking for a change to use the UINavigationController

android/src/main/kotlin/com/smileidentity/flutter/SmileIDPlugin.kt

robin-ankele

Tested the flow on iOS and Android and those seem to work fine. Left some small comments, which would be great if they could be addressed.

robin-ankele · 2025-01-13T15:55:30Z

example/ios/Runner.xcodeproj/project.pbxproj

@@ -474,7 +474,7 @@
 				CODE_SIGN_IDENTITY = "Apple Development";
 				CODE_SIGN_STYLE = Automatic;
 				CURRENT_PROJECT_VERSION = "$(FLUTTER_BUILD_NUMBER)";
-				DEVELOPMENT_TEAM = 99P7YGX9Q6;
+				DEVELOPMENT_TEAM = 5SB38K2R24;


We shouldn't be changing the development team?

I had to use my company's development account. This will be reverted before merging. I'll keep this comment open so I don't forget to revert it once it's complete.

example/ios/Flutter/AppFrameworkInfo.plist

example/lib/main.dart

- added smile_id_smart_selfie_enrollment_enhanced method - added smile_id_smart_selfie_authentication_enhanced_method

CoderNamedHendrick · 2025-01-13T20:28:25Z

Added 3 more smile id products; smart selfie authentication, enhanced enrollment and enhanced authentication.

@JNdhlovu I moved calls to the products' APIs to its implementation class which should resolve your earlier concern, do take a look. I'll be expecting your valuable feedback.

Also @robin-ankele @jumaallan

CoderNamedHendrick · 2025-01-14T06:55:08Z

@JNdhlovu I've refactored some of the setup, now client applications do not need to wrap their apps in a UINavigationController🚀

JNdhlovu

Hey @CoderNamedHendrick well done on the refactor, kindly find feedback

Orientation changes
Navigation (cancelling) as well if we'll be using an exposed activity/view controller we need to have a way of going back if needed
on android back pressing I am not getting any exception or result

Otherwise this is great work

android/src/main/kotlin/com/smileidentity/flutter/SmileIDProductsPluginApi.kt

android/src/main/AndroidManifest.xml

JNdhlovu · 2025-01-20T09:54:02Z

ios/Classes/SmileIDProductsPluginApi.swift

+
+public class SmileIDProductsPluginApi: SmileIDProductsApi {
+
+    var navigationController: UINavigationController? = nil


Same as android we need to either lock orientation or handle orientation changes

Concerning iOS, locking orientation is enforced on an app level and my attempts at locking the orientation of a single view weren't successful. The other attempts which influence the app-wide orientation state are stated here but need to evaluate if we need to do so.

I also observed in the earlier implementation the orientation wasn't fixed so do we leave setting the orientation to portrait on the developer as is currently done? @jumaallan @JNdhlovu

ios/Classes/SmileIDProductsPluginApi.swift

tobitech · 2025-01-21T11:56:37Z

Tested and Jobs run fine on iOS.

jumaallan · 2025-01-21T14:56:51Z

@CoderNamedHendrick some iOS notes

We need to lock the iOS orientation to portrait (on landscape the preview is half dark)
We need to force the theme to light mode - theming is not 100% working on iOS so we normally lock it to light mode (refer to this PR - Temporarily force the light mode even if the user is in dark mode ios#219)

CoderNamedHendrick · 2025-01-21T14:59:05Z

@CoderNamedHendrick some iOS notes

We need to lock the iOS orientation to portrait (on landscape the preview is half dark)

We need to force the theme to light mode - theming is not 100% working on iOS so we normally lock it to light mode (refer to this PR - Temporarily force the light mode even if the user is in dark mode ios#219)

Yeah @jumaallan I noticed this dark mode behaviour and forcing it to light mode is the way to go, thanks for the tip. I'll also lock the orientation in my next update.

- lock orientation on android - set preferred colorScheme to light mode on iOS

… mode

jumaallan · 2025-01-27T14:06:55Z

@JNdhlovu @tobitech @robin-ankele

There are more changes on this PR, so please take a look at this again

CoderNamedHendrick · 2025-01-27T14:36:05Z

@jumaallan I have some changes I'm currently working on that I'm yet to push. As soon as I finish within the week, it can be reviewed.

- added document verification enhanced product - added biometric kyc product

rough poc of platform views migration

5b0e16f

CoderNamedHendrick requested a review from a team as a code owner January 5, 2025 17:37

github-actions bot assigned CoderNamedHendrick Jan 5, 2025

prfectionist bot added the Review effort [1-5]: 4 label Jan 5, 2025

remove unncessary delegate controller

44e4851

CoderNamedHendrick changed the title ~~Migration from platform views~~ Migration from platform views 🚧 Jan 7, 2025