fix(dynamic-links,android): getInitialLink returned more than once, and sometimes returned null

[sharc7]

Description

This PR addresses two issue:

1. getInitialLink link is returned more than once.
2. getInitialLink returns null.

• The Android SDK doesn't seem to clear the dynamic link after calling getDynamicLink in getInitialLink.
  (as stated in the official docs: Calling getDynamicLink() retrieves the link and clears that data so it is only processed once by your app.)
  This causes the link to be retrieved even when returning to the app from the Overview screen after backgrounding the app using the back button, which leads to unwanted results such as deep links being handled more than once.
• When getInitialLink is called before the host is resumed, getCurrentActivity will be null and getInitialLink will return null instead of trying to getDynamicLink.

Related issues

Fixes #3990
Fixes #3027

Release Summary

Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • Yes
• My change supports the following platforms;
  • Android
  • iOS
• My change includes tests;
  • e2e tests added or updated in packages/\*\*/e2e
  • jest tests added or updated in packages/\*\*/__tests__
• I have updated TypeScript types that are affected by my change.
• This is a breaking change;
  • Yes
  • No

Test Plan

---

Think react-native-firebase is great? Please consider supporting the project with any of the below:

• 👉 Star this repo on GitHub ⭐️
• 👉 Follow React Native Firebase and Invertase on Twitter

🔥

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/ekhbjo153
✅ Preview: https://react-native-f-git-fork-sharc7-fix-dynamic-links-android-8f1083.invertase.vercel.app

[CLAassistant]

All committers have signed the CLA.

[codecov]

Codecov Report

Merging #4735 (18a234e) into master (a8dcd54) will increase coverage by 3.61%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4735      +/-   ##
==========================================
+ Coverage   85.48%   89.09%   +3.61%     
==========================================
  Files         109      109              
  Lines        3712     3712              
  Branches      347      347              
==========================================
+ Hits         3173     3307     +134     
+ Misses        473      363     -110     
+ Partials       66       42      -24     

[mikehardy]

Hi there! Thanks for posting this, it looks like it is solving a real problem. However I have some clarifying questions in order to understand it since the code is not immediately obvious

This PR addresses two issue:

getInitialLink link is returned more than once.

Am I correct in understanding that the first issue was already addressed but the previous solution is just incomplete for a specific condition? That condition being on app background / resume:

getInitialLink returns null.

The Android SDK doesn't seem to clear the dynamic link after calling getDynamicLink in getInitialLink.
(as stated in the official docs: Calling getDynamicLink() retrieves the link and clears that data so it is only processed once by your app.)

The language used here makes this seem like it is not handled at all but in order to understand the PR I need to be sure that I'm understanding: we already attempt to handle this firebase-ios-sdk behavior in a way react-native-firebase consumers want (get data then clear it), just that it is not working 100%. Otherwise I'm not sure why the gotInitialLink variable that existed before in the code

This causes the link to be retrieved even when returning to the app from the Overview screen after backgrounding the app using the back button, which leads to unwanted results such as deep links being handled more than once.
When getInitialLink is called before the host is resumed, getCurrentActivity will be null and getInitialLink will return null instead of trying to getDynamicLink.

Assuming I understand correctly that the module already attempts to handle it, this part is not clear to me.

So if I understand this correctly:

• if app not in background and initial link not fetched, "works" (link marked as fetched, link returned)
• if app in background and initial link already fetched, "works" (link already marked as fetched, no link returned)
• if app in background and initial link not fetched, fails - this PR fixes this? (link not marked as fetched, no link returned)

What if link is marked as fetched, app is in background, and user opens a distinct / different link? Should the initialLinkUrl itself be stored and used as the canary (instead of a gotInitialLink boolean) and then compared to see if it's different? 🤔

[sharc7]

Hi, thanks for the quick reply!

Am I correct in understanding that the first issue was already addressed but the previous solution is just incomplete for a specific condition? That condition being on app background / resume:

Yes that's correct, mostly for the special case that is "backgrounded using the back button".

The language used here makes this seem like it is not handled at all but in order to understand the PR I need to be sure that > I'm understanding: we already attempt to handle this firebase-ios-sdk behavior in a way react-native-firebase consumers want (get data then clear it), just that it is not working 100%. Otherwise I'm not sure why the gotInitialLink variable that existed before in the code

Assuming I understand correctly that the module already attempts to handle it, this part is not clear to me.

So if I understand this correctly:

if app not in background and initial link not fetched, "works" (link marked as fetched, link returned)
if app in background and initial link already fetched, "works" (link already marked as fetched, no link returned)
if app in background and initial link not fetched, fails - this PR fixes this? (link not marked as fetched, no link returned)

I'm sorry, I didn't mean to say that it's not handled at all,
just that in my experience the previous solution to the getInitialLink link is returned more than once issue was incomplete/not 100% working as it would return the initial url again when returning from the overview screen after the app is backgrounded using the back button or manually calling getInitialLink for example, in a touchable. that is - performing more than one call to getInitialLink.
Other backgrounding methods (home button or overview -> different app -> home button) worked as expected, meaning no link was returned because the host wasn't actually destroyed so getInitialLink wasn't called again when returning.

I've only managed to reproduce getInitialLink returns null issue after the onNewIntent was called and a link was returned, this sometimes took a few rounds of:

1. backgrounding using the home button
2. opening a link which resulted in the onNewIntent method to return a link (onLink)
3. backgrounding the app with the home button again
4. opening the same/different link
5. using the back button to background the app
6. finally opening the same or a different link

What if link is marked as fetched, app is in background, and user opens a distinct / different link? Should the initialLinkUrl itself be stored and used as the canary (instead of a gotInitialLink boolean) and then compared to see if it's different? 🤔

I don't think so, if the app was not backgrounded using the back button and then a link is opened, this will be handled by the onNewIntent hook (onLink).
I don't think we should compare the actual link, just that know that a call was handled successfully.
In my opinion, and from what I understand from the docs, getInitialLink should only return a link once if it exists or null if doesn't, and should return null for all future calls (because the value should be cleared after the first call), unless it's the case where the app has been backgrounded by the back button and a link launched the app, doesn't matter if it's a new or the same link, in this case (where the app is launched after a link was opened), we should try to return the link.
Other than that, I think that if the app (react-native, JS side) needs to use this value again after receiving it from react-native-firebase, it (the app) should manage this on its own.

I actually haven't thought about calling getInitialLink with a touchable before submitting the PR, so I've made some minor changes and addressed this issue with the gotInitialLink variable, I can push another commit if what I described above sounds good, or adjust according to what we discuss here before pushing.

[mikehardy]

Okay - thanks for the thoughtful reply

Can I ask you to clarify what you mean by "host"? In terms of react-native on Android I'm aware of the Android Application as a thing, I'm aware of the bridge, and I'm aware of the JS bundle running on the JS thread. When you say host, do you mean the Catalyst bridge? I think you do but I want to be clear - I don't think the actual words we use are that important I just have to make sure we're talking about the same concepts

Assuming this is correct, can you lay out (like you did in steps 1-6 above) the same or similar steps but this time also detailing for the error case exactly what state each of the 3 react-native app components is in (e.g. "Android Application Resumed, bridge up, original JS thread running", or "Android Application Paused, bridge down, JS thread destroyed", or "Android Application Resumed, bridge up, JS thread 2 running after bridge re-init" or similar)

I think you're on to something real here but I haven't probed this corner case and am not set up to reproduce it so I'll trust you on handling the corner cases etc

One thing I would like is a comment next to each member variable in the code explaining the state it is tracking, and then on the conditionals that test the new state variables, a comment explaining the state transition that we are looking for (and guarding against, basically)

[sharc7]

Can I ask you to clarify what you mean by "host"? In terms of react-native on Android I'm aware of the Android Application as a thing, I'm aware of the bridge, and I'm aware of the JS bundle running on the JS thread. When you say host, do you mean the Catalyst bridge? I think you do but I want to be clear - I don't think the actual words we use are that important I just have to make sure we're talking about the same concepts

I'm not very familiar with React Native's (or Java's) internals, the reason I mentioned "host" was because the lifecycle method referred to "host" as in onHostResume, so whichever one that refers to.

Assuming this is correct, can you lay out (like you did in steps 1-6 above) the same or similar steps but this time also detailing for the error case exactly what state each of the 3 react-native app components is in (e.g. "Android Application Resumed, bridge up, original JS thread running", or "Android Application Paused, bridge down, JS thread destroyed", or "Android Application Resumed, bridge up, JS thread 2 running after bridge re-init" or similar)

I think you're on to something real here but I haven't probed this corner case and am not set up to reproduce it so I'll trust you on handling the corner cases etc

I'm not sure how to check all of those mentioned above, if you could assist in pointing me in the right direction I'd be happy to provide information.
In the meantime, I logged getReactApplicationContext().getLifecycleState() in getInitialLink and it appears that
it sometimes returns BEFORE_CREATE or BEFORE_RESUME which is probably why getCurrentActivity() returns null at that point resulting at the method resolving with null for that case.

One thing I would like is a comment next to each member variable in the code explaining the state it is tracking, and then on the conditionals that test the new state variables, a comment explaining the state transition that we are looking for (and guarding against, basically)

Pushed a commit with some adjustments and comments as requested.

[mikehardy]

Cool - thanks for that - I'll re-queue this for review, that application state logging is key to understanding which part of the application (and what state it is in...) which should help me read through this and make sure I understand it. I mean, I think I do already but most the PRs here don't actually add state and state tracking to the system, being a glorified wrapper over an SDK makes for easy PRs normally, and I just want to make sure if I/we approve adding state that I/we get it right :-). Cheers

[sharc7]

cool, thanks!
I understand that adding state is unusual, there are probably other ways to solve this, so obviously feel free to change/discard this PR if it isn't correct, and let me know if there's anything else needed from me in this PR.
On a side note - thanks for all the hard work put into this project!

[mikehardy]

I think this is likely a case where adding state is the only way to handle it as the JS thread + bridge (the Activity in Android terms but the whole application from developer's perspective) in react-native can have divergent state from native (the Application in Android terms) so it actually makes sense, I'm looking at it from the perspective of 99% sure I'm going to merge, possibly as-is from current PR state FWIW

My pleasure on the maintenance, I do enjoy making things work

[mikehardy]

Oh - the CLA must be signed though - please click the details link on that CI failure and do the needful for it to go green

[sharc7]

Sounds good. Thanks for the detailed explanation.

Oh - the CLA must be signed though - please click the details link on that CI failure and do the needful for it to go green

Done 👍

[mikehardy]

Nice!
I had only non-semantic changes (trivial whitespace etc) - this LGTM on a re-re-read ;-)

There was an unrelated timeout / CI failure on android which is unfortunate because this affects Android code. I'm waiting for a green CI run there then I can merge this

[mikehardy]

CI went green! This should go out as 10.5.2 shortly, thanks!

[ivanstnsk]

Do you have the same behavior on iOS as well? @mikehardy @sharc7

[mikehardy]

@ivanstnsk The majority of this fix was peculiar to the interaction between android lifecycle / react-native design, with a sort of side fix of not returning the dynamic link multiple times. On iOS it is possible it still returns the same dynamic link multiple times if you call getInitialLink multiple times but I'm not 100%, have you tested it?

[ivanstnsk]

@mikehardy I've discovered exactly the same behavior on iOS - the getInitialLink is not clearing the link after handling. Tested on @react-native-firebase/dynamic-links v10.5.1

[mikehardy]

@ivanstnsk I am not surprised - I'd welcome a PR to fix it and happily work with anyone that proposes them

[sharc7]

CI went green! This should go out as 10.5.2 shortly, thanks!

Awesome, thank you! 😄 🚀