search

appcues / appcues-android-sdk

The Appcues Android SDK
https://www.appcues.com/mobile
MIT License
10 stars 3 forks source link

Prevent composition re-use at Experience level #541

Closed iujames closed 1 year ago

iujames commented 1 year ago

I believe this will be a simple fix, for a subtle and difficult to chase down issue observed in the Showcase app recently.

TLDR; we should never re-use a root level AppcuesComposition (or any of it's top level composition elements) when loading a new Experience into the same AppcuesFrameView (embed).

The Observed Issue

On the Embeds example page in the Android Showcase app, there is an embed at the top of the page and then a button to launch a tooltip. That tooltip then has a button to load a different embed into the frame at the top of the page, dismissing the existing one. It was found that sometimes (often) the loading of the second embed into the frame would fail, and we'd be left with no embed at the top of the page. Furthermore, exiting and returning to this page could lead to a situation where other embeds in the view were broken, no content loading, and seemingly no way to bring them back. Reviewing events in the debugger, it appears that the second embed was triggering experience start and step seen analytics, as if it was rendering, but just not visible (a clue for later).

Here is a video example of this issue:

https://github.com/appcues/appcues-android-sdk/assets/19266448/a14be777-edf9-4c21-b251-f3c800b12366

Findings

My first hunch was that this might be an issue with RN frame sizing logic. However, I tried it out on native first, and could reproduce the issue there. This lead me to believe it is actually an issue with something about the embed rendering flow in the core native SDK, not RN specific.

To chase this down, I ended up putting log tracing all throughout the rendering flow, to understand when the frame was hiding and showing, and when.

Upon first review, I thought perhaps there is a race condition leading to the previous embed removal happening after the next embed render, causing it to render the embed, but then hide the view. This led me to discover that the removeView call is being executed inside of a View.post here. I became suspicious that this might be the root cause and tried to remove it. This led to some cascading issues in tests, seemingly similar to this, which led to some other code changes to try to work around. Finally, was able to test it all out in the RN Showcase example, and it seemed like it might work, but eventually I could still repro the issue - it was not fixed. I'm still wondering if this post {...} usage could potentially lead to some issues (the removal happening later than expected), but I don't really think it is causing this issue.

Next, through more log tracing, I found what was actually calling he removal (out of order?) when the new Experience was supposed to be rendering. It was this line of code in the AppcuesComposition LaunchOnHideAnimationCompleted. How could this be if it was starting a new Experience? First, this should only call when the content visibility is set false (during dismissal), not during initial render. Second, it should only be called when the UIState is Dismissing, which should not be the state as it is starting a new Experience in the AppcuesViewModel 🤔

Then, it occurred to me. Compose must be re-using a previous composition here, which was picking up the last known state of the visibility and the UIState value, rather than a fresh new state. It was calling collectAsState on an older VM instance, re-using old values. This likely occurs since nothing in the arguments to AppcuesComposition change from the render attempts of the first and second embeds and it thinks it can just re-use the existing composition (and VM).

Fix

Using a key(..) { .. } around the composition that changes when the Experience changes (the instance ID), as done in this PR, forces it to use a fresh start each time. Testing this out in all scenarios I was previously able to repro the issue (native and RN) now seem to work fine.

A video of the new (expected) behavior in the Showcase app:

https://github.com/appcues/appcues-android-sdk/assets/19266448/a0456e2a-7217-4695-bf66-40eb5239b525

iujames commented 1 year ago

I'm not opposed to this change, I think it works well, but I'm wondering if maybe the issue is that the viewModel is not emitting a different state after the Dismissing which could be causing the issue.

yeah, I actually did try one update to move to Idle after dismiss like this https://github.com/appcues/appcues-android-sdk/commit/981493c9136011d06088275fd4189b1e0034b2ee . I think that could work, but I was unsure what else might be lingering around and bite us in the future unexpectedly. For instance, the locals kept for composition state here https://github.com/appcues/appcues-android-sdk/blob/main/appcues/src/main/java/com/appcues/ui/composables/CompositionRemembers.kt#L37 ?

So my thinking was that it was safest to just always start with a clean composition when rendering a new Experience instance.