Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use synchronous fragment transaction with embed frame #133

Merged
merged 2 commits into from
Jan 9, 2024

Conversation

iujames
Copy link
Contributor

@iujames iujames commented Jan 9, 2024

relates to #132

This update changes the AppcuesWrapperView implementation to use commitNow() (synchronous) instead of 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.

@iujames iujames requested a review from andretortolano January 9, 2024 14:25
Copy link

@andretortolano andretortolano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@iujames iujames merged commit ec72af7 into main Jan 9, 2024
8 checks passed
@iujames iujames deleted the fix/fragment-crash branch January 9, 2024 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants