appcues / appcues-react-native-module

The Appcues React Native Module
MIT License
11 stars 0 forks source link

use synchronous fragment transaction with embed frame #133

Closed iujames closed 10 months ago

iujames commented 10 months ago

relates to #132

This update changes the AppcuesWrapperView implementation to use [commitNow()](https://developer.android.com/reference/androidx/fragment/app/FragmentTransaction#commitNow()) (synchronous) instead of [commit()](https://developer.android.com/reference/androidx/fragment/app/FragmentTransaction#commit()) (asynchronous) - to avoid an observed timing issue where a crash like the following could occur:

java.lang.IllegalArgumentException: No view found for id 0x3f (unknown) for fragment AppcuesWrapperFragment{8fefa90} (dbc9a29b-cda2-414e-a3e0-6299daf39b32 id=0x3f tag=63)
    at androidx.fragment.app.FragmentStateManager.createView(FragmentStateManager.java:513)
    at androidx.fragment.app.FragmentStateManager.moveToExpectedState(FragmentStateManager.java:282)
    at androidx.fragment.app.FragmentManager.executeOpsTogether(FragmentManager.java:2189)
    at androidx.fragment.app.FragmentManager.removeRedundantOperationsAndExecute(FragmentManager.java:2100)
    at androidx.fragment.app.FragmentManager.execPendingActions(FragmentManager.java:2002)
    at androidx.fragment.app.FragmentManager$5.run(FragmentManager.java:524)
    at android.os.Handler.handleCallback(Handler.java:942)
    at android.os.Handler.dispatchMessage(Handler.java:99)

I believe the observed issue was due to the asynchronous nature of commit(), and some scenarios where the React Native view hierarchy is rapidly updating at the same time an AppcuesFrameView is attempting to be created in the layout. One observed scenario was a launch from push notification.

I was able to reproduce this using a test case that would cold launch the app from a deep link, and try to add an AppcuesFrameView in the layout before immediately (next line) navigating away to a new page. In the SignInScreen.js in the sample app here, I used some test code like below to accomplish this.

  const [deepLinkReceived, setDeepLinkReceived] = useState(false);

  useEffect(() => {
    const handleDeepLink = async (event) => {
      const url = event.url;
      console.log('Deep link received:', url);
      setDeepLinkReceived(true);
      navigation.navigate('Main');
    };

    // Add event listener for deep linking
    Linking.addEventListener('url', handleDeepLink);

    // Check if the app was opened with a deep link
    Linking.getInitialURL().then((url) => {
      if (url) {
        handleDeepLink({ url });
      }
    });
  }, [navigation]);

Then, later in the view, had a conditionally added AppcuesFrameView that would attempt to initialize right as the view was being torn down and navigated away.

{deepLinkReceived && <AppcuesFrameView frameID="sign-in-frame" />}

when the native implementation uses commitNow() instead, the transaction runs synchronously on the UI thread, as expected, and there is no crash in this scenario.

The other small change in AppcuesReactNativeModule is to simply remove the check for the current Activity at SDK init. It is not required to have an Activity at this moment, and quite possible in scenarios like Android push notification launch, that the Application is being started but the Activity has not yet started - it is still ok to initialize the Appcues SDK at that point.