- Notifications
You must be signed in to change notification settings - Fork3
fix: improve file sync agent picker#128
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| @@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is already checked inrefreshSessions.
| @@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This makes the form unsubmittable if any of the fields are trivially invalid.
| .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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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?Uh oh!
There was an error while loading.Please reload this page.
Coder-Desktop/Coder-Desktop/Views/FileSync/FileSyncSessionModal.swift OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Coder-Desktop/Coder-Desktop/Views/FileSync/FileSyncSessionModal.swift OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
ethanndickson commentedApr 7, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
9f625fd intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.