reactnavigation: ignore state change when getCurrentRoute() is undefined

[i1skn]

📢 Type of change

• Bugfix
• New feature
• Enhancement
• Refactoring

📜 Description

In src/js/tracing/reactnavigation.ts:

When _onStateChange is called and getCurrentRoute() is undefined,
then the reference this._latestTransaction would be cleared and next time, when state is changed and getCurrentRoute() is not undefined - it would not update the transaction with metadata from the route, cause it thinks we've already done that.

💡 Motivation and Context

This bug result in Route Change events instead of named with the actual route.

💚 How did you test it?

Just modified the code and saw that now Route Change events are called properly with the actual routes names.

📝 Checklist

• I reviewed submitted code
• I added tests to verify changes
• All tests passing
• No breaking changes

🔮 Next steps

[krystofwoldrich]

Hi,
thanks for the fix.

Could you describe simple example, when do you get undefined current route?

[krystofwoldrich]

• fix: Discard prior transactions on react navigation dispatch #2053

[i1skn]

Hi! I'm afraid I'm not sure how exactly to simply reproduce it. I have a big app and there are two general flows: for signed in and not signed users. The app spends some time to check if the user signed in and only then mounts the stack of screens. I've noticed that during this time I see two triggers of _onStateChange(). But irrespective to that, I think the logic to cancel the transaction should be inside the nested ifs, cause otherwise the comment above is wrong. If we did not enter the nested ifs - we did not handle the state change, hence transaction started in _onDispatch() is still awaiting to be populated.

[krystofwoldrich]

@i1skn Thanks for the description. I'm just making sure, that we are not changing something intentional.

[krystofwoldrich]

@i1skn Hi, I'm still not sure if the change is correct. Would you be able to make a small example app were it will show the behaviour?

Because the _onStateChange is called after state changed I'm thinking it could be valid to remove the latestTransaction because the app is in some strange state when no navigator is rendered.

Or it could be that the current route is undefined because we are out the old route but the new one is not rendered yet. But that's only my assumption and we need to check this before we make this change. That's why same small example app would helped a lot.

[i1skn]

@krystofwoldrich thanks for coming back to me.

1. As I mentioned, the app I'm working on right now is big and track why exactly _onStateChange happens twice would be quite an investigation and I'm not sure I have the resources to pull it off right now.

2. Irrespective of why _onStateChange is happening - the current behavior is incorrect. We neither cancel()/finish() the transaction, neither properly keep it. The only thing we do is we are removing _latestTransaction reference, which does not do anything to an actual transaction. Even more, this transaction still be collecting all the spans and eventually be finished after the idle time. The only thing this transaction would be deprived from - metadata about the actual route we are on, hnce this transaction would be called Route Change.

To summarize: we should either cancel the transaction or do nothing to it. Right now we just sort of prevent it from being updated, which to my opinion is incorrect. But cancelling the transaction, as we do when route has changed too quickly is dangerous here, cause at least in my case I will loose app_start_warm/cold spans.

Let me know your thoughts on this, thanks!

[krystofwoldrich]

@i1skn What you're saying make sense to me.

Could you share a link to transaction, where the name is not assigned.

[krystofwoldrich]

@i1skn Thank for the details you send privately. It took me a while to grasp what should be the correct behaviour.