we can increase readability by refactoring the "/namespaces/" + ro.resource.namespace + "/" + ro.resource.type + "s/" + ro.resource.name so we don't repeat this particular string so many times here

this is causing a console error that is hiding the real problems :)

🤦‍♂️ Making things worse for myself. Thanks for catching that.

we still want to access the list pages (/namespaces, /deployments, /pods) etc. from somewhere so let's make sure these don't get lost

Good point. I wasn't sure whether those pages would be needed since the sidebar partially "becomes" the namespace page. Although, I do agree that we still want to be able to view stats about requests between namespaces since we lose visibility of these stats if we rely solely on the sidebar.

we should include some way to access these list pages in the nav

authorities are kind of different from the other resources and it makes less sense to have this type of dashboard for them; also they're not linked in the web server routes to going to http://localhost:8084/namespaces/linkerd/authoritys/10.1.1.97:9995 won't go anywhere. We should filter out the resource types that we don't have resource detail pages for (or add routing for them in web/srv/server.go, currently there's only three resource types there)

if there are no sub items in the menu, we should either omit or disable linking for the high level component (e.g. there aren't any deployments when you select namespace kube-system and so the nave is misleading; it takes a lot of time to click on each resource type and figure out that there aren't resources in it)

Good point. I will get that set up on my next commit.

extra spaces

you can get this by using processMultiResourceRollup:

import { processMultiResourceRollup } from './util/MetricUtils.js';
...
let statsByResource = processMultiResourceRollup(allRsp)

The object format is a little different, so you'd be needing to do r.name instead of r.resource.name in all the places you do r.resource, for example.

Oh nice! this is great. Thanks for pointing that out.

I realize this is existing code, but we can also get this by using processSingleResourceRollup:

        let nsStats = processSingleResourceRollup(nsRsp);
        let namespaces = _(nsStats).map('name').sortBy().value();

ns.nsValue is a little repetitive, maybe ns.labelValue and ns.labelName? or simply ns.name and ns.value?
_.map(namespaces, label => ... label.name .... label.value might also be a clearer option

We could get rid of the resources we don't want to show when we process the metrics in loadFromServer. This could just be delete resourceGroupings["authority"] there, for example. This removes the need for the deleting of it in this method.

I'll also note that services aren't wired in, so you might want to handle that too.

Per discussion on the ticket, I think we'll want to retain the ability to collapse (or hide) the sidebar.

we have a similar method to this in like three places in the codebase now. I think we should try to consolidate this in a helper function in MetricUtils.js

Yea good call. We probably should make the method accept a map that are associated status labels to good, ok and bad so we can use arbitrary css classNames.

I think this would be more readable as

export const excludeResourcesFromRollup = (rollupMetrics, resourcesToExclude) => {
  _.each(resourcesToExclude, resource => {
    delete rollupMetrics[resource];
  })

Font feedback: looking at the sidebar we can see that the subitems are larger than the headings Deployments and Pods. Esp since the pod names are long, I think we should use a smaller font size (maybe 12px?)

nit: _.isEmpty(sidebarComponents[resourceName])

readability nit:

    let namespaces = [
      {value: "all", name: "All Namespaces"}
    ].concat(_.map(this.state.namespaces, ns => {
      return { value: ns, name: ns };
    }));

if we're only using one _ function here, there's no need to wrap o and then unwrap it with value... so this could be written as return _.filter(o, r => r.namespace === namespace && r.added)

is this comment outdated?

the only difference between these is this line... what if we refactored the repeated code by putting the filter into a named variable?

  filterResourcesByNamespace(resources, namespace) {
    let filter = namespace === "all" ? r => r.added :
      r => r.added && r.namespace === namespace;

    return _.mapValues(resources, res => _.filter(res, filter));
  }

Slick! I like it!

do we need this if here if we've filtered out the unadded components in filterResourcesByNamespace?

sweeeet

nit: trailing `

sweeet! 🎉

spacing nit:

else {
  return successRateLabels.good;
}

TIOLI: I think it would help usability to add a title={`${r.namespace}/${r.name}`} here so that the user can hover over and see the full name in case the name is truncated (probably not super important, but could be nice to have).

Makes total sense and I feel it is necessary. Thanks for pointing that out! 🙏

sorry I didn't catch this before, but title tags contain text only and ignore tags within the element.
https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/title

But also, unlike the pod names, I don't think there's any case where the full Resource Title will be cut off in the sidebar as is. So I think we should remove this title= entirely.

I can do that in a small branch of misc fixes I'm working on.

Ah! thanks for catching that. I wasn't aware that the title prop was going to be the page title. I assumed it was something that antd used for displaying the Menu.SubMenu. Thanks for taking care of this!

oh, blah, wrong link, I meant the attribute... buries face
https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/title