Build breadcrumbs from right to left where possible

[skateman]

Breadcrumbs mostly follow the tree hierarchy and @rvsia builds the related tree any time a breadcrumb requires it. This is not very effective and there are also issues when the tree has hidden leaves or it's lazily loadable.

My proposal is to define a reverse TreeBuilder#has_kids_for for breadcrumbs for the reverse path in the hierarchy for building just the breadcrumbs. This way we would no longer depend on the ˙TreeBuilder˙ when we need just a few breadcrumbs.

Something like:

breadcrumb_parent MiqAeInstance { |p| p.miq_ae_class }
breadcrumb_parent MiqAeClass { |p| p.miq_ae_namespace }

[skateman]

@miq-bot assign @skateman

[rvsia]

How I said, we could do this only for trees which are lazy loaded. 🌴 But I am not sure which ones they are (or if every tree is lazyloaded..)

[skateman]

The following explorer trees are lazyloaded:

TreeBuilderReportSavedReports
TreeBuilderReportReports
TreeBuilderReportSchedules
TreeBuilderReportDashboards
TreeBuilderReportWidgets
TreeBuilderReportRoles
TreeBuilderChargebackReports
TreeBuilderServices
TreeBuilderServiceCatalog
TreeBuilderCatalogItems
TreeBuilderOrchestrationTemplates
TreeBuilderCatalogs
TreeBuilderImages
TreeBuilderStorage
TreeBuilderStoragePod
TreeBuilderPxeServers
TreeBuilderPxeCustomizationTemplates
TreeBuilderPxeImageTypes
TreeBuilderIsoDatastores
TreeBuilderConfigurationManager
TreeBuilderConfigurationManagerConfiguredSystems
TreeBuilderAlertProfile
TreeBuilderAutomationManagerProviders
TreeBuilderAutomationManagerConfigurationScripts
TreeBuilderAeClass
TreeBuilderUtilization
TreeBuilderOpsRbac
TreeBuilderOpsVmdb

And also in Ivanchuk, so backporting #6002 and #6019

TreeBuilderAutomationManagerConfiguredSystems
TreeBuilderConfigurationManagerConfiguredSystems
TreeBuilderStoragePod

[skateman]

1. So after I went through the 🌳s and as we have a lot of hash nodes, it's not possible to do a child-to-parent reversed build without major changes in how TreeBuilder works. This would add a level of complexity that I'm not comfortable with to have in our codebase, not even mentioning an ivanchuk backport. On the other hand, sending the tree hierarchy twice from the controllers and in general, building the tree twice is fundamentally wrong.

2. My other idea was to retrieve the hierarchy from the rendered node and propagate it using RxJS. Adding the functionality to the breadcrumbs was easy and it works well, but we have to find all the places to emit these messages. I'm still thinking about this how can we solve it with the least invasive set of changes, but to be honest, it's not very promising. There is a lot of added complexity in the current frontend implementation of the trees compared with the times I created it. For example the Generic Objects tree is a complete 🍝 and does handles everything differently.

3. I liked @himdel's idea about not showing the tree node structure in the breadcrumbs at all. The current breadcrumbs are a new feature, so there would be no regression compared with older versions. It is also a clean and highly performant solution as we would not build the trees twice, but I guess it needs a lot of approvals.

4. The ideal solution would be to have the whole tree stored in Redux and connect the breadcrumb component to this store. It would be the easiest and cleanest solution, however, we're pretty far away from it and it's not something for backporting.

For now I see option 3 as the most viable solution with the future possibility of implementing option 4, but I'm still trying hard with option 2 in the meantime.

[martinpovolny]

So we need to sort the issues into:

1. what really is a regression
2. what's inconsistency or just not yet perfect coverage by the breadcrumbs
3. also we might need to separate the issues that are related to GO because that is just too different :-( However any GO issues should not be regression as that just could have never worked.

Looking forward we need 2 types of fixes:

1. minimal change fixes for the regressions
2. systematic, no/low tech debt generic solution for all the trees (with the possible exception of GO if that's just too much a mess).

I suggest by starting collecting all the BZs/issues here and sorting them regression yes/no. That should give us an estimate about the size of fixes 1).

Meanwhile we can discuss fixes 2).

Most importantly: we need:

1. A complete plan.
2. Finish this.

[skateman]

@martinpovolny we're talking about a single issue here, if you're trying to jump navigate into an explorer, the tree hierarchy is not reflected in the breadcrumbs when rendered for the first time.

[skateman]

The most minimal change is option 3, i.e. not displaying the tree hierarchy in the breadcrumbs...

[martinpovolny]

The most minimal change is option 3, i.e. not displaying the tree hierarchy in the breadcrumbs...

Yes, that is my suggestion for Ivanchuk: Do not display the tree hierarchy, because:

• it is broken (inconsistent with lazy loading)
• cannot be quickly fixed
• not having it is not a regression (it was never there)

Instead display the breadcrumbs up to the open accordion and then the active node breadcrumb (item).

For master (future release) the hierarchy will be displayed. For that a proper solution based on calculation the path through the tree from leaf to the root is needed.

[skateman]

Okay so we need a way to retrieve parent node info from the child node using TreeBuilder. This isn't that problemmatic for nodes that are being mapped from actual db records. We can just simply use something like I mentioned above.

The problem is hitting us with hash nodes, some of them are necessary, but we're kinda overusing them for records with custom attributes where we could avoid this. We have to go through all the hash nodes defined for explorer TreeBuilder classes and find which one can be converted to record-based, like here. The ones where we can't convert, my guess is that they're all dynamic and we can move them out to constants. I really need to go through all of them to be sure about this. However, if this is true, we can work with the constant definitions in both upwards and downwards in the tree hierarchy. If some of them are dynamic, we can still work with them, but there might be some performance considerations to think about.

[miq-bot]

This issue has been automatically marked as stale because it has not been updated for at least 6 months.

If you can still reproduce this issue on the current release or on master, please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions!

[skateman]

@miq-bot rm_label stale

[miq-bot]

This issue has been automatically marked as stale because it has not been updated for at least 3 months.

If you can still reproduce this issue on the current release or on master, please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the traige process documentation.

[Fryguy]

@skateman Is this still relevant?

[skateman]

Yes, explorer trees are still using the old stuff.

[miq-bot]

This issue has been automatically marked as stale because it has not been updated for at least 3 months.

If you can still reproduce this issue on the current release or on master, please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.