chore: added latency tooltips on workspaces #134
Conversation
| } | ||
| } | ||
| if (LastHandshake != null) | ||
| description.Append($"\n\nLast handshake: {LastHandshake?.ToString() ?? "Unknown"}"); |
There was a problem hiding this comment.
| description.Append($"\n\nLast handshake: {LastHandshake?.ToString() ?? "Unknown"}"); | |
| description.Append($"\n\nLast handshake: {LastHandshake.ToString()}"); |
|
|
||
| // For compatibility with older deployments, we assume that if the | ||
| // last ping is null, the agent is healthy. | ||
| var isLatencyAcceptable = agent.LastPing != null ? agent.LastPing.Latency.ToTimeSpan() < HealthyPingThreshold : true; |
There was a problem hiding this comment.
Did you check that the behavior of the C# protobuf library does this? They usually just use a zero value when it's not set to be consistent with other protobuf implementations
| if (string.IsNullOrEmpty(tooltip)) | ||
| return "No connection information available."; |
There was a problem hiding this comment.
Could this just be an else on the if (p2p) {} else if (derp) {} at the top?
| You're connected peer-to-peer. | ||
| You ↔ {Latency.Value.Milliseconds} ms ↔ {WorkspaceName} | ||
| """ | ||
| ); | ||
| } | ||
| else if (PreferredDerpLatency != null) | ||
| { | ||
| description.Append($""" | ||
| You're connected through a DERP relay. |
There was a problem hiding this comment.
On Mac, we include a (High latency) after both first sentences if the ping is unacceptable, otherwise the status icon turns yellow with no indication as to why.
| You're connected through a DERP relay. | ||
| We'll switch over to peer-to-peer when available. | ||
| Total latency: {PreferredDerpLatency.Value.Milliseconds} ms |
There was a problem hiding this comment.
Shouldn't this be the total latency, not just the preferred derp latency?
| """ | ||
| ); | ||
|
|
||
| if (PreferredDerp != null && Latency != null) |
There was a problem hiding this comment.
Shouldn't this second null check be against PreferredDerpLatency?
Fixes: #106