Open
Conversation
Prevent duplicate trackInAppOpen calls when the activity is recreated (e.g. rotation) by saving/restoring the inAppOpenTracked flag via onSaveInstanceState. Also preserve static singleton state during configuration changes to match Fragment version behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tests cover singleton lifecycle (create/getInstance/dismiss), dialog show/dismiss behavior, URL click handling, layout variants (fullscreen/top/bottom/center), duplicate display rejection, and resilience to resize after dismiss. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The static notification/clickCallback/location fields can be read from background threads (e.g. processMessages after network sync) while being written on the main thread during show/dismiss. Mark them volatile to ensure cross-thread visibility in both the Fragment and Dialog in-app notification classes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
franco-zalamena-iterable
requested review from
davidtruong,
joaodordio and
sumeruchat
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
franco-zalamena-iterable
changed the title
- Replace Java 16 pattern matching instanceof with traditional instanceof + cast for broader JDK compatibility - Make InAppTrackingService initialization lazy in InAppServices to avoid capturing IterableApi.sharedInstance at class load time Made-with: Cursor
All implementors and consumers are within the same package, so public visibility is unnecessary and widens the public API surface. Made-with: Cursor
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The volatile keyword was added to the notification/clickCallback/location static fields on the Fragment path, but these are only mutated from the main thread (Fragment lifecycle callbacks). The modifier added a memory barrier with no observable race to guard against, and touched shipping code unnecessarily. Restoring the fields to their pre-PR (master) state. Made-with: Cursor
Two try/catch blocks were silently swallowing any exception from IterableApi.getInAppManager() and InAppManager.getMessageById(), dropping the error without logging. The Fragment path's processMessageRemoval() does neither and is the reference behavior, so InAppTrackingService now matches: direct calls, no try/catch. The defensive "inAppManager == null" guard was also dead code because IterableApi.getInAppManager() is annotated @nonnull and throws RuntimeException on null — a contract the Fragment path already relies on. The companion test removeMessage_shouldNotCrash_whenInAppManagerIsNull was verifying an impossible scenario (Mockito stubbed @nonnull to return null, which Kotlin's inserted null-check then caught via the old try/catch); it's removed here. Made-with: Cursor
A plain android.app.Dialog does not participate in the host Activity's save-state pipeline the way DialogFragment does. Dialog.show() always invokes onCreate(null) via dispatchOnCreate(null), and our code path never calls Dialog.onRestoreInstanceState, so the savedInstanceState read in onCreate was unreachable. The onSaveInstanceState override was also never invoked (nothing wires it into the Activity state save). On config change the Dialog is auto-dismissed, dismiss() clears the static singleton, and any subsequent createInstance() produces a fresh instance with inAppOpenTracked=false — same as when the bundle dance was present. The in-memory inAppOpenTracked flag is kept as a defensive re-entry guard inside onCreate. Made-with: Cursor
createInstance previously mutated the static clickCallback/location fields and overwrote the notification singleton before any duplicate check. The "already showing" check lived only in IterableInAppDisplayer.showIterableDialogNotificationHTML, so any direct caller of createInstance (which is @JvmStatic public) could clobber an in-flight dialog's callbacks. The guard is now inside createInstance: if a notification instance already exists it is returned unchanged and statics are untouched. Statics are also only assigned after successful construction, so a constructor throw cannot leave them in a partial state. The outer Displayer guard remains as defense-in-depth. Made-with: Cursor
The previous setupBackPressHandling branched on whether the host was a ComponentActivity and either registered an OnBackPressedCallback on the Activity's dispatcher or fell back to setOnKeyListener. While a Dialog is showing, back-key events go to the Dialog's window first, not the Activity — so the OnBackPressedDispatcher callback wouldn't fire for the interesting case anyway, and registering on the Activity's dispatcher can reorder against the Compose host's own BackHandlers. Unifying on setOnKeyListener handles back for every Activity subtype, including ComponentActivity, and avoids that dispatcher ordering risk. The backPressedCallback field and its dismiss() cleanup are removed, and the ComponentActivity/OnBackPressedCallback imports are dropped. Made-with: Cursor
The Dialog path was doing a plain alpha fade (300ms) regardless of in-app layout, while the Fragment path uses layout-specific animations at 500ms: slide_down_custom / fade_in_custom / slide_up_custom for entry and top_exit / fade_out_custom / bottom_exit for exit. This commit closes that gap for Compose hosts without touching the shipping Fragment class. Changes: - InAppAnimationService now exposes getEnterAnimationResource(layout) and getExitAnimationResource(layout), plus a hideAndAnimateWebView method symmetric with showAndAnimateWebView. - showAndAnimateWebView now takes the layout and loads the correct animation with ITERABLE_IN_APP_ANIMATION_DURATION (500ms). - The hardcoded ANIMATION_DURATION_MS=300 was conflating two distinct timings: background transitions (300ms) and view animations (500ms). It's now replaced by the matching IterableConstants values. - IterableInAppDialogNotification.hideWebView plays the exit animation, hides the background, and dismisses after 400ms — mirroring the Fragment's hideWebView timing. When shouldAnimate=false it still dismisses synchronously so existing Dialog tests remain valid. - runResizeScript carries a TODO(future PR) pointing at the native window resize logic that still needs porting from the Fragment's resize(float). Until then, Dialog hosts rely on the HTML's window.resize() self-sizing hook, which covers fixed-height in-apps but not dynamically-resizing content. Made-with: Cursor
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
🔹 Jira Ticket(s) if any
✏️ Description
Why?
The SDK's in-app display requires FragmentActivity, which pure Compose apps don't use. This adds a Dialog-based fallback that activates automatically when the host activity is a ComponentActivity.