Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: skip api submission #76

Merged
merged 4 commits into from
Nov 20, 2024
Merged

feat: skip api submission #76

merged 4 commits into from
Nov 20, 2024

Conversation

JNdhlovu
Copy link
Contributor

Story: https://app.shortcut.com/smileid/story/14361/update-wrapper-sdks-with-latest-native-updates

Summary

  • Update wrapper SDKs with latest native updates

Known Issues

WIP: Fix android serialization

Test Instructions

test with skipApiSubmission true should not submit the job , false or not set should submit as per default for products

  1. Enrollment
  2. Auth
  3. Doc V
  4. Enhanced DocV

Screenshot

N/A

@prfectionist
Copy link

prfectionist bot commented Nov 18, 2024

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

API Change
New skipApiSubmission flag added that changes core API submission behavior. Need to verify this works as expected across all SDK components.

Event Type Change
Changed from RCTDirectEventBlock to RCTBubblingEventBlock for onResult callback. Need to verify event propagation still works correctly.

@prfectionist
Copy link

prfectionist bot commented Nov 18, 2024

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible bug
Provide default values for nullable boolean parameters to prevent potential null pointer exceptions

Initialize showAttribution and showInstructions with their default values when
passing them to SmartSelfieAuthentication to avoid potential null issues.

android/src/main/java/com/smileidentity/react/views/SmileIDSmartSelfieAuthenticationView.kt [27-28]

-showAttribution = showAttribution,
-showInstructions = showInstructions,
+showAttribution = showAttribution ?: true,
+showInstructions = showInstructions ?: true,
Suggestion importance[1-10]: 8

Why: The suggestion addresses a potential null safety issue by providing default values for nullable boolean parameters, which could prevent runtime crashes.

8
Performance
Optimize performance by preventing unnecessary object recreation during UI recomposition

Move the customViewModelStoreOwner declaration outside the setContent block to avoid
recreating it on each recomposition.

android/src/main/java/com/smileidentity/react/views/SmileIDSmartSelfieEnrollmentView.kt [19-21]

+val customViewModelStoreOwner = CustomViewModelStoreOwner()
 setContent {
-  val customViewModelStoreOwner = CustomViewModelStoreOwner()
   CompositionLocalProvider(LocalViewModelStoreOwner provides customViewModelStoreOwner) {
Suggestion importance[1-10]: 7

Why: Moving the CustomViewModelStoreOwner initialization outside the setContent block prevents unnecessary object recreation during recomposition, improving performance.

7
Maintainability
Remove unnecessary trailing whitespace to maintain clean code formatting

Remove the trailing empty line that was added to avoid unnecessary whitespace in the
file.

android/src/main/java/com/smileidentity/react/views/SmileIDView.kt [115-116]

 }
-+
Suggestion importance[1-10]: 1

Why: The suggestion only addresses a minor formatting issue with no impact on functionality or maintainability.

1

@@ -11,6 +11,7 @@
13B07FBC1A68108700A75B9A /* AppDelegate.mm in Sources */ = {isa = PBXBuildFile; fileRef = 13B07FB01A68108700A75B9A /* AppDelegate.mm */; };
13B07FBF1A68108700A75B9A /* Images.xcassets in Resources */ = {isa = PBXBuildFile; fileRef = 13B07FB51A68108700A75B9A /* Images.xcassets */; };
13B07FC11A68108700A75B9A /* main.m in Sources */ = {isa = PBXBuildFile; fileRef = 13B07FB71A68108700A75B9A /* main.m */; };
1EE4A9FA2CEB682800995931 /* smile_config.json in Resources */ = {isa = PBXBuildFile; fileRef = 1EE4023C2B066384003A8717 /* smile_config.json */; };

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to commit the smile_config.json file into the project?

@@ -248,6 +248,7 @@
isa = PBXResourcesBuildPhase;
buildActionMask = 2147483647;
files = (
1EE4A9FA2CEB682800995931 /* smile_config.json in Resources */,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this as well, should have left a comment here.

Copy link

@tobitech tobitech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good on iOS!

@JNdhlovu JNdhlovu merged commit 98f3565 into main Nov 20, 2024
6 checks passed
@JNdhlovu JNdhlovu deleted the feature/skipApiSubmission branch November 20, 2024 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants