QuickEditor: Make sure the QE works well when the app get's killed during the OAuth flow

[AdamGrzybkowski]

Closes #525

Description

The current code has a problem with the OAuth flow when the app get's killed by the system (or when the "Don't keep activities" setting is ON). Here's how Hector described it:

...

The culprit of the problem is that we add the OnNewIntentListener in the OAuthPage so while that composable and the activity where it's contained is alive there is no problem.

However, when the activity is destroyed after opening the CustomTab, it will be recreated after the OAuthFlow has finished. In the MainActivity example, when the OAuthFlow finishes, the steps are:

MainActivity.onCreate()
MainActivity.onNewIntent()
The onCreate method recreates the activity as it was, keeping the QE with the OAuthScreen opened, but the onNewIntent is called before the composables are inflated and the listener in the OAuthPage composable registered, so we are missing it.

We can, somehow, try to resolve this in an activity under our control (GravatarQuickEditorActivity), but I think it is more complicated in a third-party app as (MainActivity).

So far, the main QE's flow would make the third-party activity handle the deep linking for the OAuth flow, so there was very little we could do to fix this. Hector explored a suggestion that maybe we could achieve this by using Deep Linking via Jetpack Compose Navigation but our NavGraph lives only in the Bottom Sheet, not the activity so making it work seems equally tricky.

This PR proposes a different approach that not only fixes this issue #525 but also removes the requirement from third-party apps to use singleTask launch mode on the activity that launches the QuickEditor.

The solution:

Instead of relying on third-party activity to handle the OAuth deep links, the QuickEditor opens a fully transparent activity GravatarOAuthActivity before launching the Custom Tab to start the OAuth. From now on this is the only Activity that requires the launchMode=singleTask. This activity also handles the onNewIntent and we no longer need to handle that within the Composable so the whole timing issue described by Hector is no longer a problem.

I had to set a local flag to make sure the GravatarOAuthActivity is finished when the Custom tab is dismissed (without it the QE would appear as focused but wouldn't actually work because it would be under the activity).

Additionally, I had to add SavedStateHandle to the OAuthViewModel so as not to send the StartOauth action in the init block. When the activity is recreated, so is the ViewModel, so we need to make sure that action hasn't been emitted before.

Here's the result:

oauth_flow_after.mp4

Once this is merged we can deprecate the GravatarQuickEditorActivity as its only purpose was so that third parties don't have to set singleTask on their Activity. We will have to adjust the docs as well.

Testing Steps

1. Go to the Developer settings and enable the "Don't keep activities" - you can also play by killing the app via ADB
2. Fresh install the Demo app
3. Launch the QE
4. Dismitt the custom tab via x in the Toolbar
5. Confirm you see the QE with the intro page
6. Tap Continue
7. Tap Deny
8. Confirm the QE was closed and error toast was shown
9. Launch the QE
10. Tap Approve
11. Confirm you were logged in and you can see your Avatars
12. Disable the "Don't keep activities" setting
13. Repeat the above steps, and make sure all works as expected

[wpmobilebot]

📲 You can test the changes from this Pull Request in Gravatar Demo by scanning the QR code below to install the corresponding build.

	App Name	Gravatar Demo
	Commit	6385df6
	Direct Download	gravatar-demo-prototype-build-pr541-6385df6.apk

[mlumeau]

I was able to trigger a NullPointerException here by opening the customtab url in a browser tab, finishing the flow, and going back to that tab later on which tried to open the app again through the deep link.

Is there a way for us to handle these edge cases more gracefully?

[AdamGrzybkowski]

I was able to trigger a NullPointerException here by opening the customtab url in a browser tab, finishing the flow, and going back to that tab later on which tried to open the app again through the deep link.

Hmm I'm not sure I got the steps correctly. Can you clarify what do you mean by by opening the customtab url in a browser tab? Or better, post a screen recording.

Is there a way for us to handle these edge cases more gracefully?

We could close the Activity - it can only work if those params are set.

[mlumeau]

Here's a screen recording, sorry for the French language, but I think it's explicit:

Screen_recording_20250122_145251.mp4

Here you can see at the end that I'm going back to Firefox, refresh the page (which happens automatically if I open Firefox from the launcher) and the demo app flashes and closes down abruptly, I have this stack trace in the log:

FATAL EXCEPTION: main (Ask Gemini)
Process: com.gravatar.demoapp, PID: 16745
java.lang.RuntimeException: Unable to start activity ComponentInfo{com.gravatar.demoapp/com.gravatar.quickeditor.ui.oauth.GravatarOAuthActivity}: java.lang.NullPointerException
	at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:4129)
	at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:4316)
	at android.app.servertransaction.LaunchActivityItem.execute(LaunchActivityItem.java:222)
	at android.app.servertransaction.TransactionExecutor.executeNonLifecycleItem(TransactionExecutor.java:133)
	at android.app.servertransaction.TransactionExecutor.executeTransactionItems(TransactionExecutor.java:103)
	at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:80)
	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2719)
	at android.os.Handler.dispatchMessage(Handler.java:109)
	at android.os.Looper.loopOnce(Looper.java:232)
	at android.os.Looper.loop(Looper.java:317)
	at android.app.ActivityThread.main(ActivityThread.java:8787)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:591)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:871)
Caused by: java.lang.NullPointerException
	at com.gravatar.quickeditor.ui.oauth.GravatarOAuthActivity.onCreate(GravatarOAuthActivity.kt:33)
	at android.app.Activity.performCreate(Activity.java:9019)
	at android.app.Activity.performCreate(Activity.java:8997)
	at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1528)
	at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:4111)

[AdamGrzybkowski]

Ohh ok, now it's clear. You go back to the wp.com OAuth site after successfully finishing the flow early. Yeah, I think what I mentioned before will work for this. This is a weird edge case, thanks for testing!

[AdamGrzybkowski]

I pushed the changes to fix this NPE.

[AdamGrzybkowski]

@mlumeau I pushed the commit that makes this change backward-compatible. You can test it by applying this patch
manifest.patch - this will make the third-party activity handle the onNewIntent.

If you change nothing, you can verify this still works as expected and this warning log Setup GravatarOAuthActivity in your AndroidManifest.xml to handle the OAuth flow. is not logged.

[AdamGrzybkowski]

@mlumeau I removed the enter transition to be on the safe side. I don't think it was visible because of the Custom tab but this should get rid of it regardless. Sorry for the last minute update.

[mlumeau]

LGTM! 🚀