I'm finding this quite strange. Why not only return from the API null or undefined? Having the API returning both types look strange to me.

Yeah, this seems like it was meant to be double-bookkeeping, but with how the server types are set up, thenull conditionals won't ever trigger. TheomitEmpty JSON tag will make sure that if the values ofhost_ip orhost_port are missing, they'll always evaluate toundefined

👍 Will remove the extraneousnull checks.

Two questions:

• Is there a reason why this code is using the MUI version of the tooltip, rather than the one we have in our components folder?
• Is the span here doing anything? I could see needing it to change some flexbox behavior, but I don't see anything obvious that would require it

Is there a reason why this code is using the MUI version of the tooltip, rather than the one we have in our components folder?

Copy-pasta, most likely!

Is the span here doing anything? I could see needing it to change some flexbox behavior, but I don't see anything obvious that would require it

The tooltip wouldn't display if the link was disabled; the<span> was a random StackOverflow find.

Is the value ofhasHostBind likely to change over time? If I'm reading this right, it feels weird to me that we could be rendering out a link that is always disabled. At that point, why make it an interactive element in the first place?

It could change if the user bound the remote port to a local port and rebuilt the container. My reasoning for including the non-interactive variants was that if they simply didn't show up in the UI, it would be more confusing to users. Having it show up with a tooltip explaining why it was not enabled felt "right" to me.

Yeah, I think that's a solid solution. I guess I'm just usually a little more skittish about disabled UI elements for accessibility reasons. But if/when that becomes a priority, I think it should be easy to update the component

Why are we making a non-nullable assertion forhost_port, when just a few lines above, we're checking whether it's defined? I'd rather do one of two things:

• UpdateportForwardURL to acceptundefined values
• Update the render logic to render something else out if the value is missing

Update the render logic to render something else out if the value is missing

Do you have any suggestions for an alternative "nowhere" URL? If there is no host port, we can't forward in the Coder UI. Maybeabout:blank?

Defaulting href to an empty string if the link is disabled.

location.protocol should always be eitherhttps orhttp, right?

Correct.

@BrunoQuaresma Have we switched over to Shad for link variants like this?

It depends. I would not expect BE folks migrating the components to be honest, but in this case I think would make more sense to replace them by the new button component like this:

<ButtonasChild><ahref="..."onClick={...}/></Button>

Oh wait, sorry – got mixed up by the diff. Didn't realize the link was already here

I'd suggest we make this a follow-up?

Yeah, that's totally fine with me