AppRoutingEffects' async initialization leads to race conditions, clearing URL params

[psybuzz]

In #4197, we discovered a race condition and fixed it
in a non-ideal manner.

Background:

when both AppRoutingEffects and CoreEffects initialize, they make some changes asynchronously; former makes modification to URL (with two debounce/nextTick) and latter makes XHR to fetch plugins listing. Nothing guarantees that the XHR to land after the URL is modified and the test did reveal the cases when the XHR finished before router getting initialized.

from #4197 (comment)

The symptom is that after opening TensorBoard, the '#scalars' hash may get dropped
from the URL immediately. Any actions that update the hash with a key=value, e.g.
changing smoothing (adds &smoothingWeight=... to URL) causes TensorBoard to
show "No active plugin".

The PR proposes a fix where AppRouting preserves any URL hash during its initial
'navigated' action. However, it would be more ideal if AppRouting itself could
manage the URL hash entirely.

Alternatively, HashStorageContainer could be modified to wait for AppRouting
to initialize, before HashStorageContainer sets '#scalars' in the URL hash.
However, for this to land, it requires all TB embedders to use AppRouting.

Googlers, see mirrored issue b/169799696

[psybuzz]

It turns out that this bug also affects URL queryParams, not just the hash. AppRouting assumes it manages the URL queryParams entirely and can safely discard existing params because it is the source of truth.

Discarding initial queryParams turns out to break things like ?tensorboardColab=, which are read directly from window.location.search at some arbitrary time during the TB session (see tensorboard/plugins/scalar/tf_scalar_dashboard/tf-scalar-card.ts). This leads to TB thinking that the ?tensorboardColab= param was not present, causing bad behavior in Colab.

There are a handful of other instances where we read directly from window.location.search at a time that is possibly after the AppRoutingEffects initialization.

[stephanwlee]

I don't want to say our currently approach generally is a bad one; I think this is an unfortunate byproduct of our usage of query parameter in unorganized way.

Of course in the long run, we can do what we do with the activePlugin and bootstrap the redux state correctly based on the initial query parameter but until all of our offering uses the router and redux state, it will not happen.

For now, I think we need to find a workaround one of which can be populating some environment variable on the global scope (window) in the deep link state.

[psybuzz]

As per offline discussion, we'd like to add a regression test that adds ?tensorboardColab=true to the initial URL, do stuff in the app, and make sure no POST requests fire. This might not be possible with the webdriver API, but we can likely spy on the test http client.

[pindinagesh]

@psybuzz

Could you kindly look at this PR and confirm if this can be closed? Thanks!

[psybuzz]

Thanks for checking in, sounds reasonable to close. I haven't been familiar with TensorBoard frontend updates for a while, so if someone wants to reopen this to track adding a test, please do.