Repository navigation
fix: treat time since last in-app as unbounded when no in-app was dismissed - #2814
RaphaelFakhri wants to merge 2 commits into
Conversation
abdulraqeeb33
left a comment
There was a problem hiding this comment.
The null fallback is the right sentinel. Elapsed time is compared in milliseconds, and Long.MAX_VALUE is large enough that a greater-than threshold matches immediately and a less-than threshold does not. For a positive threshold the follow-up offset stays negative, and the method returns before scheduling a timer. start() copies the persisted dismissal time in before trigger subscribers run, so null means there is no stored dismissal.
Both new tests passed. The red test check is the coverage-comment step: this PR is from a fork, and the workflow token got a 403 creating the issue comment. The unit-test task and the coverage threshold both passed. That 403 also fails the aggregate build check.
One edge on the new sentinel: a negative min_time_since value of about -0.002 seconds or below wraps requiredTimeInterval - Long.MAX_VALUE to a large positive delay (DynamicTriggerController.kt offset subtraction). Timer.schedule then throws IllegalArgumentException ("Illegal execution time") after DynamicTriggerTimer has already started a non-daemon timer thread. suspendify logs that and drops the evaluation pass. The previous 999999 sentinel stayed on the offset <= 0 return. Positive thresholds, which are what this fix is for, still take that return. Worth a guard if a negative value can show up in a payload.
|
Ran The two tests on the PR:
Four extra probes, left out of the branch:
|
With no in-app message dismissed, the time since the last one is Long.MAX_VALUE, so a negative required interval overflowed the offset into a large positive delay and Timer.schedule threw. Return before computing the offset when the interval has already passed.
|
Thanks for the probes. I added the guard in 4783cc7. I couldn't run the Gradle unit tests here because this machine has no Android SDK, so CI is the first run of the new test. |
Description
One Line Summary
Fix "Time since last in-app message" triggers that wait instead of firing when no in-app message has been dismissed yet.
Details
Motivation
Fixes #1602. When
lastTimeInAppDismissedis null,DynamicTriggerControllerfalls back toDEFAULT_LAST_IN_APP_TIME_AGO, which is meant to mean "a very long time ago". The constant is999999, but the value is compared against intervals in milliseconds, so it only represents about 16.7 minutes.As a result, a trigger such as "time since last in-app message greater than 3600 seconds" does not fire for a user who has never dismissed an in-app message. Instead, the SDK schedules a timer for the remaining 43 minutes or so, and the message shows late or not at all if the session ends first. Any threshold above about 16.7 minutes is affected.
The fallback is now
Long.MAX_VALUE, so a user who has never dismissed an in-app message always satisfies a "greater than" threshold, and never satisfies a "less than" threshold.Scope
Only the fallback used when no in-app message has been dismissed changes. Triggers evaluated after a dismissal still use the persisted dismissal time, and session-time triggers are unchanged.
Testing
Unit testing
Added
DynamicTriggerControllerTests:Manual testing
Not tested on a device. The change only affects the trigger evaluation covered by the unit tests.
Affected code checklist
Checklist
Overview
Testing
Final pass