fix(ui): only use visible and enabled tabs for selected tab and routing in entity profiles

[Masterchen09]

This PR fixes a bug when the first tab in an entity profile is not visible and/or is not enabled, but is selected by default when the url does not specify a tab. It also prevents an invisible and/or disabled tab from being manually selected by modifing the url (e.g. here the Queries tab is disabled, but is selected by adding "/Queries" to the url).

As I have noticed this bug after PR #5864, I have also moved the Datasets tabs after the Charts tab (the Charts and Datasets tab are conditionally visible or not, therefore the Datasets tab should be the first tab, when the Charts tab is not visible).

Checklist

• The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
• Links to related issues (if applicable)
• Tests for the changes have been added/updated (if applicable)
• Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
• For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

[github-actions]

Unit Test Results (build & test)

636 tests  ±0   632 ✔️ ±0   15m 58s ⏱️ +13s
164 suites ±0       4 💤 ±0 
164 files   ±0       0 ❌ ±0 

Results for commit abfb483. ± Comparison against base commit 729e486.

♻️ This comment has been updated with latest results.

[jjoyce0510]

@Masterchen09 Hey there. Does this change tabs so that you'll never see a disabled state tab?

I'm debating whether we want to kill disabled tabs in their entirety or not. I think having disabled tabs can be useful to indicate that DataHub supports additional features, but they just happen to not be active..

What do you think?

[Masterchen09]

This does not change the visibility of the disabled tabs in the tab list, the disabled tabs are still visible in the tab list and cannot be selected as in the past.

The PR is only changing the routing logic which is applied: Currently it is possible to view a disabled/invisible tab by manually modifying the url, but as the tabs are disabled/invisible I think this should not be possible, because the tabs might be disabled/invisible for a good reason (most of the time there is no content in it).

The second change is the default routing logic which is applied when no tab is selected, for example when the entity profile is opened from the browse page: In this case a disabled/invisible tab should never be selected as the default tab, even if the disabled/invisible or invisible tab is the first tab in the tab list (again, there might be a good reason for hiding/disabling it).

[jjoyce0510]

The full tab set still contains all of them, whether they are enabled or not.

What do you think about removing the changes to this file, and only changing the logic inside EntityTabs which changes the default selected tab? I do think that might be a bit simpler, with less duplication of logic.

[jjoyce0510]

One more minor thing then I think we can get it. I really don't like repeated the visibleTabs, enabledVisibleTabs logic in both places

[Masterchen09]

I am not really sure about your comments, therefore I will try to summarise/understand it:

The changes to the EntityProfile ensure that only visible and enabled tabs are routed: routedTab contains undefined when there is no tab to be routed (e.g. if last part of the URL is missing) or if the tab is invisible and/or disabled, because only visible and enabled tabs are passed to useRoutedTab. I am not sure about your opinion here, because in the first comment you suggest to remove the changes in EntityProfile, but in the second comment you say it is fine if we find a solution for the repeated logic (maybe I understand something wrong here, but I am not a native speaker 😇). In my opinion invisible and/or disabled should not be routed, because they are invisible/disabled for a good reason (most of the time no data would be available for these tabs, but one could also imagine that wrong data is displayed instead, a developer who disables the tab because of this reason expects that the tab is not accessible, but currently it is...I know this is only a hypothetical scenario, but it could be possible).

The changes to the EntityTabs are related to the selection of the default tab, when routedTab contains undefined. However we have to pass the loading property to the EntityTab from the EntityProfile, because otherwise the routing is taking place before we know which tabs can be routed (the set of visible and enabled tabs is only known once the data was loaded, because it might be influenced by the data), therefore the changes to EntityProfile regarding this are needed anyway. Regarding the duplicated logic I think it is not that easy to answer: We have to decide whether the EntityTabs component should be responsible that only visible tabs are going to be shown or if the component should expect to be feed only with visible tabs. In the first case we need to have both arrays (visibleTabs and enabledAndVisibleTabs) and the duplicated logic in EntityTabs, because the UnborderedTabs component needs both enabled and disabled (but visible) tabs, but for the routing we need only the visible and enabled tabs. In the second case we could do the filtering of the visible tabs in EntityProfile and only pass the visible tabs to EntityTabs then. In my opinion the first case is better because the EntityTabs component knows the best what to do with the tabs and it ensures by itself that everything is correct. In the first case it would be possible to simplify the logic in EntityProfile by removing the visibleTabs array and only have the enabledAndVisibleTabs array there, but this depends on whether we remove the changes to EntityProfile or not:

const enabledAndVisibleTabs = tabs.filter((tab) => tab.display?.visible(entityData, baseEntity) && tab.display?.enabled(entityData, baseEntity));

(Sorry for the long comment at the beginning I thought it would be short, but once I thought about it loud, it got a bit longer. 😁)

[Masterchen09]

Something else which came to my mind since yesterday: Instead of passing the loading state as a prop to the EntityTabs, we could add it to the EntityContext. As the entityData and the baseEntity are also part of the EntityContext are needed for the routing logic, it would make sense to have the loading state there aswell.

Alternatively the routing logic in EntityTabs could be moved into the EntityProfile, then we would only need the visible tabs in EntityTabs (and we don't need to pass the loading status to EntityTabs then). The disadvantage of this approach is that the EntityProfile does not only contain the EntityTabs and we would need some more logic there to only execute the routing logic when the EntityTabs is visible (I think the routing logic in EntityTabs should stay in EntityTabs).

[jjoyce0510]

So what if we can keep the logic in here for routing only to visible tabs, but simply say that if the "selected tab name" is not in the "enabledAndVisibleTabs names", then fall back to the first tab in "enabledAndVisibleTabs".

If we do this, we may not need to check loading at all, since the enabledAndVisibleTabs will be re-computed each time the EntityData changes!

How does that work?

[Masterchen09]

I could not test it yet, but I think this approach should technically work. In theory, there could be two potential problems. With this approach and without respecting the loading state we would do the routing twice: One routing takes place before loading the data and the other after loading the data.

From the browser's session history point of view this shouldn't be a problem, because when using useRouteToTab the state is replaced in the history and not appended. The only thing here is that a tab which will not be visible/accessible after loading the data might be visible for a short amount of time after the first routing (the tab itself will be visible because the EntityProfile is rendering the selected tab independently from the loading state of the entity, but before the routing takes place no tab is visible because the selected tab is undefined). However, I think this is something we can safely ignore.

The other problem is more theoretical, but I don't think that we should really ignore it: Assuming we have two tabs for an entity and the first tab is not visible before the loading of the data, but after and the second tab is visible before and after, then the first tab will not be the default tab, because the selected tab will not be undefined after the first routing and therefore the routing logic would not be applied anymore. The question here is whether this is fine or if we want to intentionally delay the routing here because of this. If we want to delay it, we would need the loading state in EntityTabs. Maybe we should then also render a placeholder as long the selected tab is undefined or alternatively to be consistent with EntityTabs (when the selected tab is undefined the first tab seems to be automatically selected by the component, see also here: ant-design/ant-design#39730) render always the first visible tab in EntityProfile (but not route to this tab in this case, to be able to apply the "show the first visible and enabled tab as default after loading"-logic).

If you are agree that we should respect the loading state and delay the routing in EntityTabs (I don't think there is another solution for this?), I will try to prepare something by moving the loading state into the EntityContext (even if we currently only need the loading state in EntityTabs it could also be useful in the tabs themselves at one place or another).

edit: I had now the possibility to test your approach and my theoretical second example is not only theoretical because it also affects disabled tabs. In case of a dashboard the Chart tab is disabled before the data is loaded, because without the data there are also no charts. Then the second tab (Documentation) is selected and is routed during the first routing, after loading the data, the documentation tab remains selected.

[Masterchen09]

@jjoyce0510 I have updated the PR: The loading variable is now passed to the EntityContext and is retrieved along with the data via the useEntityData hook. Passing an empty string to the activeKey prop of the Tabs component when the selectedTab is undefined prevents the first visible tab from being selected automatically, therefore there is no need for an placeholder (no tab is selected in the Tabs component -> no tab is rendered in the EntityProfile).

[jjoyce0510]

Nice!

[jjoyce0510]

Okay one more small question.

Why not just pass "visibleTabs" into the EntityTabs component?

Then the EntityTabs component will always only be aware of visible tabs. This makes sense since other tabs will not be involved in rendering. Then, it can properly route the routedTab (if it is not undefined AND it is enabled -- we can keep that check in the EntityTabs component)

In short, the EntityProfile will filter for tabs which are visibile at all, while the child component, EntityTabs, will filter for tabs which are enabled.

It keeps the responsibilities fairly clearly separated, to avoid having both places check both visible and enabled statuses.

[Masterchen09]

This is something I was not really sure about it, but now only visibleTabs are passed to EntityTabs.

[jjoyce0510]

Here we can just remove any checks for visible tabs - only visible tabs make there way in.

We can just say "enabledTabs". Then we will route to the first enabled tab down below.

[jjoyce0510]

Starting to wonder if this 'loading' is even necessary at all.

This useEffect will run whenever it's inputs change. In this case, selectedTab will change once a new Tab is selected (e.g. the visible / enabled tabs change.

Therefore, you shouldn't really need to watch the loading parameter of the entity, right? It can be made a bit simpler?

[Masterchen09]

We have to delay the first routing (when selectedTab is still undefined) until the data is loaded, if we would not delay it, the "wrong" tab could be selected after the data was loaded (see also the edit of my previous wall of text 😇).

[jjoyce0510]

A few final comments. I think we are very close here

[jjoyce0510]

This looks great! I'm very happy with the changes.

Thanks for all the hard work here and for sharing the rich detail (like the screen capture video).

LGTM!