chore: instantly dismiss modal instead of async animation dismissal #739
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Part of: https://linear.app/customerio/issue/MBL-355/ios-fix-in-app-message-displayed-on-wrong-page There is a scenario where a modal message can be displayed on the wrong screen due to a timing issue. Although the time window where this could happen is small, it was encountered during QA testing. Test case to reproduce: * In-app message with page rule "Dashboard" sent to device. User on "Dashboard" screen. In-app message begins to render. * Similar to what is explained [here](https://customerio.slack.com/archives/C06H7MMLH6C/p1716391939537599), if the user of the app clicks a button to begin navigating to another screen in the app and the modal message finishes rendering during the screen transition animation, the modal will be displayed and will remain displayed after the animation is complete and the next screen is shown. Solution: Dismiss the modal when the screen transition animation ends and a new page rule is triggered. Ensure that when a Modal message is shown, it does not trigger an automatic screenview event. This allows the SDK to recognize user navigation to a new screen accurately. Testing: * Automated tests added to verify auto screenview tracking. * QA test verified that when a Gist Modal message shown, an automatic screenview event not tracked.
Part of: https://linear.app/customerio/issue/MBL-355/ios-fix-in-app-message-displayed-on-wrong-page When the page route changes in the SDK, we need to have the current modal dismissed and ready to display a new message. Currently, the code dismisses the modal with an animation, so it's an async operation. To reduce edge cases in the code, modify the dismissing of messages to be a synchronous/instant operation. Testing: * In sample app, I manually triggered the cancel/dismiss code to confirm that the modal dismisses instantly and when I change screens, I expect that a new message is able to show. Note to reviewers * This is the behavior implemented in the Android SDK.
Sample app builds 📱Below you will find the list of the latest versions of the sample apps. It's recommended to always download the latest builds of the sample apps to accurately test the pull request.
|
SDK binary size reports 📊SDK binary size of this PRSDK binary size diff report between this PR and the main branch |
Ahmed-Ali
approved these changes
Jun 14, 2024
There was a problem hiding this comment.
Overall nothing stood out except the logic to dismiss the in-app on page rule change.
Are there scenarios where a page rule can match more than one page?
If the answer is yes, then please don't merge till we discuss it.
Edit: For reference, Levi addressed the above question in #740
This was referenced Jun 14, 2024
Base automatically changed from
levi/ignore-cio-autoscreenviews
to
levi/inapp-strict-pagerules
June 17, 2024 13:18
…/customerio-ios into levi/instantly-dismiss-modals
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## levi/inapp-strict-pagerules #739 +/- ##
==============================================================
Coverage ? 60.81%
==============================================================
Files ? 140
Lines ? 3958
Branches ? 0
==============================================================
Hits ? 2407
Misses ? 1551
Partials ? 0 ☔ View full report in Codecov by Sentry. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of: https://linear.app/customerio/issue/MBL-355/ios-fix-in-app-message-displayed-on-wrong-page
When the page route changes in the SDK, we need to have the current modal dismissed and ready to display a new message. Currently, the code dismisses the modal with an animation, so it's an async operation. To reduce edge cases in the code, modify the dismissing of messages to be a synchronous/instant operation.
Testing:
Note to reviewers
Based on PR #732
Full chain of PRs as of 2024-06-13
levi/instantly-dismiss-modals➔levi/ignore-cio-autoscreenviewslevi/ignore-cio-autoscreenviews➔levi/inapp-strict-pageruleslevi/inapp-strict-pagerules➔main