fix: improve file sync agent picker #128
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ethanndickson
commented
Apr 3, 2025
| @@ -107,9 +107,7 @@ struct FileSyncConfig<VPN: VPNService, FS: FileSyncDaemon>: View { | |||
| // When the Window is visible, poll for session updates every | |||
| // two seconds. | |||
| while !Task.isCancelled { | |||
| if !fileSync.state.isFailed { | |||
There was a problem hiding this comment.
This is already checked in refreshSessions.
| @@ -55,15 +55,16 @@ struct FileSyncSessionModal<VPN: VPNService, FS: FileSyncDaemon>: View { | |||
| Button("Cancel", action: { dismiss() }).keyboardShortcut(.cancelAction) | |||
| Button(existingSession == nil ? "Add" : "Save") { Task { await submit() }} | |||
| .keyboardShortcut(.defaultAction) | |||
| .disabled(localPath.isEmpty || remotePath.isEmpty || chosenAgent == nil) | |||
There was a problem hiding this comment.
This makes the form unsubmittable if any of the fields are trivially invalid.
Comment on lines
119
to
127
| .task { | ||
| // If there's a file sync session error, an icon will be displayed | ||
| // next to the file sync button. The file sync window polls more | ||
| // frequently when it's open. | ||
| while !Task.isCancelled { | ||
| await fileSync.refreshSessions() | ||
| try? await Task.sleep(for: .seconds(15)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Previously, we were only polling if the file sync window was open. We want the user to be aware of any errors at a glance, so we need to poll even when the window is closed.
There was a problem hiding this comment.
Pull Request Overview
This PR improves the file sync agent picker by switching from selecting an entire Agent struct to only using the hostname, ensuring that changes in Agent status do not inadvertently unselect the agent in an active session.
- Adds an asynchronous task in VPNMenu to periodically refresh file sync sessions.
- Updates FileSyncSessionModal to use a hostname string (chosenAgent) rather than the entire Agent struct for selection.
- Removes a condition in FileSyncConfig to always refresh sessions regardless of fileSync.state.isFailed.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Coder-Desktop/Coder-Desktop/Views/VPN/VPNMenu.swift | Adds a task to poll file sync sessions every 15 seconds. |
| Coder-Desktop/Coder-Desktop/Views/FileSync/FileSyncSessionModal.swift | Refactors agent selection to work with hostnames and updates UI bindings accordingly. |
| Coder-Desktop/Coder-Desktop/Views/FileSync/FileSyncConfig.swift | Removes the condition that skipped refreshing sessions when a failure was detected. |
Comments suppressed due to low confidence (2)
Coder-Desktop/Coder-Desktop/Views/FileSync/FileSyncConfig.swift:109
- Removing the check for fileSync.state.isFailed may cause unnecessary refresh operations even when the file sync state indicates a failure. Consider reinstating or refining the condition to avoid redundant network calls.
while !Task.isCancelled {
Coder-Desktop/Coder-Desktop/Views/FileSync/FileSyncSessionModal.swift:11
- [nitpick] The variable 'chosenAgent' could be more descriptive, for example, renaming it to 'chosenAgentHost' to clearly indicate that it holds a hostname.
@State private var chosenAgent: String?
deansheather
reviewed
Apr 4, 2025
deansheather
reviewed
Apr 7, 2025
Coder-Desktop/Coder-Desktop/Views/FileSync/FileSyncSessionModal.swift
Outdated
Show resolved
Hide resolved
deansheather
approved these changes
Apr 7, 2025
Coder-Desktop/Coder-Desktop/Views/FileSync/FileSyncSessionModal.swift
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, the agent/workspace picker when creating a file sync session had the user choose between instances of the
Agentstruct. This meant the value would get unselected were the status of the agent to change. I'm not sure why I had the picker select the entire struct instead of just the hostname.