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

Update Partner Config without Relaunching Demo App #279

Merged
merged 1 commit into from
Jan 15, 2025

Conversation

tobitech
Copy link
Contributor

Story: https://app.shortcut.com/smileid/story/xxx

Summary

  • Change StateObject to ObservedObject so that injected config can re-render the view.

Known Issues

N/A.

Test Instructions

  • Launch the Demo app and switch to the Settings.
  • Tap on "Update Smile Config"
  • Provide partner config with a string or QR code
  • Switch back to Home Tab to see the current partner config info.
  • You should see the update without having to relaunch the app.
  • Please also test the jobs to be sure that this change doesn't break anything on your device. (Jobs are working fine on mine).

Screenshot

If applicable (e.g. UI changes), add screenshots to help explain your work.

@tobitech tobitech self-assigned this Jan 15, 2025
@tobitech tobitech requested a review from a team as a code owner January 15, 2025 11:43
@prfectionist
Copy link

prfectionist bot commented Jan 15, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

State Management
Changing from @StateObject to @ObservedObject could lead to unexpected view reloads and potential memory management issues if the parent view recreates the HomeViewModel frequently. Validate that the parent view maintains a stable reference to the config.

@prfectionist
Copy link

prfectionist bot commented Jan 15, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Best practice
Use appropriate property wrapper for view model lifecycle management in SwiftUI views

Consider keeping @StateObject instead of @ObservedObject for the view model.
@StateObject is more appropriate for view models owned by a view as it maintains the
object's lifecycle throughout the view's lifetime, while @ObservedObject might lead
to unexpected reinitializations.

Example/SmileID/Home/HomeView.swift [7]

-@ObservedObject var viewModel: HomeViewModel
+@StateObject var viewModel: HomeViewModel
Suggestion importance[1-10]: 9

Why: The suggestion correctly identifies that reverting from @ObservedObject to @StateObject is crucial for proper view model lifecycle management, as @StateObject ensures the view model persists throughout the view's lifetime and prevents unexpected reinitializations.

9

Copy link

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 7afc9fd

Copy link

@robin-ankele robin-ankele left a comment

Choose a reason for hiding this comment

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

Tested the flow and created two selfie enrolments with two different partners where I switched partner config between enrolments. Works fine. Thanks @tobitech - great work.

@tobitech tobitech merged commit 6d2a23c into main Jan 15, 2025
4 checks passed
@tobitech tobitech deleted the fix-update-demo-app-config branch January 15, 2025 12:02
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.

2 participants