Skip to content

feat: add remote directory picker to file sync #73

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

deansheather
Copy link
Member

@deansheather deansheather commented Apr 22, 2025

Adds a new remote directory picker window used when creating a file sync to select the remote directory.

NVIDIA_Overlay_LplyNOug3n.mp4

TODOs:

  • Use a dropdown for picking workspace agent in the file sync UI, currently it's typed out (and will crash if empty lol)
  • Fix reactivation of the window, try to make it function like any other system dialog window

Closes #27

Adds a new remote directory picker window used when creating a file sync
to select the remote directory.
@deansheather deansheather marked this pull request as draft April 22, 2025 14:56
@deansheather
Copy link
Member Author

It's ready for review, just need to finish the few TODOs

@deansheather deansheather marked this pull request as ready for review April 23, 2025 03:45
@@ -1,7 +1,6 @@
using System.Diagnostics;
using Coder.Desktop.App.Models;
using Coder.Desktop.App.Services;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
using Coder.Desktop.App.Services;
using Coder.Desktop.App.Services;
using Coder.Desktop.CoderSdk.Coder;


private readonly ISyncSessionController _syncSessionController;
private readonly IRpcController _rpcController;
private readonly ICredentialManager _credentialManager;
private readonly IAgentApiClientFactory _agentApiClientFactory;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't smell right to me---this is not a direct dependency of the FileSyncListViewModel, it's a dependency of the directory picker. Shouldn't we be using dependency injection to resolve the dependency at the time the directory picker is created, not the time time file sync list is created?

{
new()
{
Name = "(root)",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the Windows native file explorer, this is rendered as "🖥️ This PC" --- I think just 🖥️ would look a little more friendly and nice than (root).

TextTrimming="CharacterEllipsis"
IsTextTrimmedChanged="TooltipText_IsTextTrimmedChanged"
Margin="0,0,0,10" />
<ProgressRing
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice touch

[RelayCommand]
private void StartCreatingNewSession()
{
ClearNewForm();
// Ensure we have a fresh hosts list before we open the form.
SetAvailableHostsFromRpcModel(_rpcController.GetState());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a little annoying to find that the available hosts only updates when you click the button to create a new session. I could imagine starting up a workspace, then creating a new session to find your workspace isn't yet listed. To get it to populate you need to cancel out, then wait for it to start, maybe by watching the tray menu, then create a new sync session.

@@ -534,25 +538,44 @@ private void StartDaemonProcess()
Directory.CreateDirectory(_mutagenDataDirectory);
var logPath = Path.Combine(_mutagenDataDirectory, "daemon.log");
var logStream = new StreamWriter(logPath, true);
try
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you refactor this into a separate PR? It doesn't have anything to do with the remote picker.

_daemonProcess?.Dispose();
_logWriter?.Dispose();
_daemonProcess = null;
_logWriter = null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we throw an exception starting the daemon, the logStream never gets turned into _logWriter and will leave open the logfile, which makes retries fail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add remote folder picker to file sync GUI
2 participants