Skip to content

Commit 8c5a07f

Browse files
spikecurtisspikecurtis
committed
apply review suggestions
1 parent 5378529 commit 8c5a07f

File tree

5 files changed

+48
-61
lines changed

5 files changed

+48
-61
lines changed

‎App/App.xaml.cs‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -195,16 +195,16 @@ public void OnActivated(object? sender, AppActivationArguments args)
195195
return;
196196
}
197197

198-
try
199-
{
200198
// don't need to wait for it to complete.
201-
_ = _uriHandler.HandleUri(protoArgs.Uri);
202-
}
203-
catch (System.Exception e)
199+
_uriHandler.HandleUri(protoArgs.Uri).ContinueWith(t =>
204200
{
205-
_logger.LogError(e, "unhandled exception while processing URI coder://{authority}{path}",
206-
protoArgs.Uri.Authority, protoArgs.Uri.AbsolutePath);
207-
}
201+
if (t.Exception != null)
202+
{
203+
_logger.LogError(t.Exception,
204+
"unhandled exception while processing URI coder://{authority}{path}",
205+
protoArgs.Uri.Authority, protoArgs.Uri.AbsolutePath);
206+
}
207+
});
208208

209209
break;
210210

‎App/Services/RdpConnector.cs‎

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,11 @@ public struct RdpCredentials(string username, string password)
1212
public readonly string Password = password;
1313
}
1414

15-
public interface IRdpConnector : IAsyncDisposable
15+
public interface IRdpConnector
1616
{
1717
public const int DefaultPort = 3389;
1818

19-
public Task WriteCredentials(string fqdn, RdpCredentials credentials, CancellationToken ct = default);
19+
public void WriteCredentials(string fqdn, RdpCredentials credentials);
2020

2121
public Task Connect(string fqdn, int port = DefaultPort, CancellationToken ct = default);
2222
}
@@ -26,13 +26,13 @@ public class RdpConnector(ILogger<RdpConnector> logger) : IRdpConnector
2626
// Remote Desktop always uses TERMSRV as the domain; RDP is a part of Windows "Terminal Services".
2727
private const string RdpDomain = "TERMSRV";
2828

29-
public Task WriteCredentials(string fqdn, RdpCredentials credentials, CancellationToken ct = default)
29+
public void WriteCredentials(string fqdn, RdpCredentials credentials)
3030
{
3131
// writing credentials is idempotent for the same domain and server name.
3232
Wincred.WriteDomainCredentials(RdpDomain, fqdn, credentials.Username, credentials.Password);
3333
logger.LogDebug("wrote domain credential for {serverName} with username {username}", fqdn,
3434
credentials.Username);
35-
return Task.CompletedTask;
35+
return;
3636
}
3737

3838
public Task Connect(string fqdn, int port = IRdpConnector.DefaultPort, CancellationToken ct = default)
@@ -73,9 +73,4 @@ public Task Connect(string fqdn, int port = IRdpConnector.DefaultPort, Cancellat
7373

7474
return mstscProc.WaitForExitAsync(ct);
7575
}
76-
77-
public ValueTask DisposeAsync()
78-
{
79-
return ValueTask.CompletedTask;
80-
}
8176
}

‎App/Services/UriHandler.cs‎

Lines changed: 31 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111

1212
namespace Coder.Desktop.App.Services;
1313

14-
public interface IUriHandler : IAsyncDisposable
14+
public interface IUriHandler
1515
{
1616
public Task HandleUri(Uri uri, CancellationToken ct = default);
1717
}
@@ -24,10 +24,16 @@ public class UriHandler(
2424
{
2525
private const string OpenWorkspacePrefix = "/v0/open/ws/";
2626

27-
internal class UriException(string title, string detail) : Exception
27+
internal class UriException : Exception
2828
{
29-
internal readonly string Title = title;
30-
internal readonly string Detail = detail;
29+
internal readonly string Title;
30+
internal readonly string Detail;
31+
32+
internal UriException(string title, string detail) : base($"{title}: {detail}")
33+
{
34+
Title = title;
35+
Detail = detail;
36+
}
3137
}
3238

3339
public async Task HandleUri(Uri uri, CancellationToken ct = default)
@@ -52,7 +58,7 @@ private async Task HandleUriThrowingErrors(Uri uri, CancellationToken ct = defau
5258

5359
logger.LogWarning("unhandled URI path {path}", uri.AbsolutePath);
5460
throw new UriException("URI handling error",
55-
$"URI with path {uri.AbsolutePath} is unsupported or malformed");
61+
$"URI with path '{uri.AbsolutePath}' is unsupported or malformed");
5662
}
5763

5864
public async Task HandleOpenWorkspaceApp(Uri uri, CancellationToken ct = default)
@@ -63,7 +69,7 @@ public async Task HandleOpenWorkspaceApp(Uri uri, CancellationToken ct = default
6369
if (components.Length != 4 || components[1] != "agent")
6470
{
6571
logger.LogWarning("unsupported open workspace app format in URI {path}", uri.AbsolutePath);
66-
throw new UriException(errTitle, $"Failed to open {uri.AbsolutePath} because the format is unsupported.");
72+
throw new UriException(errTitle, $"Failed to open '{uri.AbsolutePath}' because the format is unsupported.");
6773
}
6874

6975
var workspaceName = components[0];
@@ -73,41 +79,38 @@ public async Task HandleOpenWorkspaceApp(Uri uri, CancellationToken ct = default
7379
var state = rpcController.GetState();
7480
if (state.VpnLifecycle != VpnLifecycle.Started)
7581
{
76-
logger.LogDebug("got URI to open workspace {workspace}, but Coder Connect is not started", workspaceName);
82+
logger.LogDebug("got URI to open workspace '{workspace}', but Coder Connect is not started", workspaceName);
7783
throw new UriException(errTitle,
78-
$"Failed to open application on {workspaceName} because Coder Connect is not started.");
84+
$"Failed to open application on '{workspaceName}' because Coder Connect is not started.");
7985
}
8086

81-
Workspace workspace;
82-
try
83-
{
84-
workspace = state.Workspaces.Single(w => w.Name == workspaceName);
85-
}
86-
catch (InvalidOperationException) // Single() throws this when nothing matches.
87-
{
88-
logger.LogDebug("got URI to open workspace {workspace}, but the workspace doesn't exist", workspaceName);
87+
var workspace = state.Workspaces.FirstOrDefault(w => w.Name == workspaceName);
88+
if (workspace == null) {
89+
logger.LogDebug("got URI to open workspace '{workspace}', but the workspace doesn't exist", workspaceName);
8990
throw new UriException(errTitle,
90-
$"Failed to open application on workspace {workspaceName} because it doesn't exist");
91+
$"Failed to open application on workspace '{workspaceName}' because it doesn't exist");
9192
}
9293

93-
Agent agent;
94-
try
95-
{
96-
agent = state.Agents.Single(a => a.WorkspaceId == workspace.Id && a.Name == agentName);
97-
}
98-
catch (InvalidOperationException) // Single() throws this when nothing matches.
99-
{
100-
logger.LogDebug("got URI to open workspace/agent {workspaceName}/{agentName}, but the agent doesn't exist",
94+
var agent = state.Agents.FirstOrDefault(a => a.WorkspaceId == workspace.Id && a.Name == agentName);
95+
if (agent == null) {
96+
logger.LogDebug("got URI to open workspace/agent '{workspaceName}/{agentName}', but the agent doesn't exist",
10197
workspaceName, agentName);
98+
// If the workspace isn't running, that is almost certainly why we can't find the agent, so report that
99+
// to the user.
100+
if (workspace.Status != Workspace.Types.Status.Running)
101+
{
102+
throw new UriException(errTitle,
103+
$"Failed to open application on workspace '{workspaceName}', because the workspace is not running.");
104+
}
102105
throw new UriException(errTitle,
103-
$"Failed to open application on workspace {workspaceName}, agent {agentName} because it doesn't exist.");
106+
$"Failed to open application on workspace '{workspaceName}', because agent '{agentName}' doesn't exist.");
104107
}
105108

106109
if (appName != "rdp")
107110
{
108111
logger.LogWarning("unsupported agent application type {app}", appName);
109112
throw new UriException(errTitle,
110-
$"Failed to open agent in URI {uri.AbsolutePath} because application {appName} is unsupported");
113+
$"Failed to open agent in URI '{uri.AbsolutePath}' because application '{appName}' is unsupported");
111114
}
112115

113116
await OpenRDP(agent.Fqdn.First(), uri.Query, ct);
@@ -137,14 +140,9 @@ public async Task OpenRDP(string domainName, string queryString, CancellationTok
137140
if (!string.IsNullOrEmpty(username))
138141
{
139142
password ??= string.Empty;
140-
await rdpConnector.WriteCredentials(domainName, new RdpCredentials(username, password), ct);
143+
rdpConnector.WriteCredentials(domainName, new RdpCredentials(username, password));
141144
}
142145

143146
await rdpConnector.Connect(domainName, ct: ct);
144147
}
145-
146-
public ValueTask DisposeAsync()
147-
{
148-
return ValueTask.CompletedTask;
149-
}
150148
}

‎Tests.App/Services/RdpConnectorTest.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ public class RdpConnectorTest
1111
[Test(Description = "Spawns RDP for real")]
1212
[Ignore("Comment out to run manually")]
1313
[CancelAfter(30_000)]
14-
public async Task ConnectToRdp()
14+
public async Task ConnectToRdp(CancellationToken ct)
1515
{
1616
var builder = Host.CreateApplicationBuilder();
1717
builder.Services.AddSerilog();
@@ -21,7 +21,7 @@ public async Task ConnectToRdp()
2121
var rdpConnector = (RdpConnector)services.GetService<IRdpConnector>()!;
2222
var creds = new RdpCredentials("Administrator", "coderRDP!");
2323
var workspace = "myworkspace.coder";
24-
await rdpConnector.WriteCredentials(workspace, creds);
25-
await rdpConnector.Connect(workspace);
24+
rdpConnector.WriteCredentials(workspace, creds);
25+
await rdpConnector.Connect(workspace, ct: ct);
2626
}
2727
}

‎Tests.App/Services/UriHandlerTest.cs‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -27,12 +27,6 @@ public void SetupMocksAndUriHandler()
2727
uriHandler = new UriHandler(logger, _mRpcController.Object, _mUserNotifier.Object, _mRdpConnector.Object);
2828
}
2929

30-
[TearDown]
31-
public async Task CleanupUriHandler()
32-
{
33-
await uriHandler.DisposeAsync();
34-
}
35-
3630
private Mock<IUserNotifier> _mUserNotifier;
3731
private Mock<IRdpConnector> _mRdpConnector;
3832
private Mock<IRpcController> _mRpcController;
@@ -51,6 +45,7 @@ public void AgentAndWorkspaceFixtures()
5145
{
5246
Id = ByteString.CopyFrom(0x1, 0x0),
5347
Name = "workspace1",
48+
Status = Workspace.Types.Status.Running,
5449
};
5550

5651
modelWithWorkspace1 = new RpcModel
@@ -73,8 +68,7 @@ public async Task Mainline(CancellationToken ct)
7368

7469
_mRpcController.Setup(m => m.GetState()).Returns(modelWithWorkspace1);
7570
var expectedCred = new RdpCredentials("testy", "sesame");
76-
_ = _mRdpConnector.Setup(m => m.WriteCredentials(agent11.Fqdn[0], expectedCred, ct))
77-
.Returns(Task.CompletedTask);
71+
_ = _mRdpConnector.Setup(m => m.WriteCredentials(agent11.Fqdn[0], expectedCred));
7872
_ = _mRdpConnector.Setup(m => m.Connect(agent11.Fqdn[0], IRdpConnector.DefaultPort, ct))
7973
.Returns(Task.CompletedTask);
8074
await uriHandler.HandleUri(input, ct);

0 commit comments

Comments
 (0)