- Notifications
You must be signed in to change notification settings - Fork3
fix: manually upgrade system extension#158
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
888fd0e to428495eCompareThere 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 fixes the manual upgrade process for the network extension and improves the handling of system extension requests. Key changes include:
- Updating the UI to display a reconfiguration message for unconfigured network extensions.
- Refactoring the system extension delegate and protocol definitions for more robust asynchronous handling.
- Adjusting error messaging and property initialization to support the manual upgrade flow.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Views/VPN/VPNState.swift | Adds a new UI branch for unconfigured network extension errors. |
| Views/VPN/VPNMenu.swift | Introduces a “Reconfigure VPN” button when the VPN state indicates a network extension issue. |
| VPN/VPNSystemExtension.swift | Refactors protocol definitions and updates system extension state handling logic. |
| VPN/VPNService.swift | Changes system extension delegate from an optional to a forced unwrapped value with initialization in init. |
| VPN/NetworkExtension.swift | Improves error logging and messaging when the network extension fails to save preferences. |
Comments suppressed due to low confidence (1)
Coder-Desktop/Coder-Desktop/VPN/VPNSystemExtension.swift:166
- Confirm that switching from using 'bundleShortVersion' to 'bundleVersion' is intended and that the version comparison logic works accurately in both development and release builds.
logger.info("Replacing \(request.identifier) v\(existing.bundleVersion) with v\(`extension`.bundleVersion)")Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -22,6 +22,35 @@ enum SystemExtensionState: Equatable, Sendable { | |||
| } | |||
| } | |||
| let extensionBundle: Bundle = { | |||
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 was previously a computed variable (the body was reran each time it was used), now it's just a lazy static constant. The body here is unchanged.
| @@ -38,7 +42,7 @@ struct VPNState<VPN: VPNService>: View { | |||
| .padding(.horizontal, Theme.Size.trayInset) | |||
| .padding(.vertical, Theme.Size.trayPadding) | |||
| .frame(maxWidth: .infinity) | |||
| default: | |||
| case (.connected, true): | |||
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.
Same behaviour, just more explicit.
Uh oh!
There was an error while loading.Please reload this page.
428495e to828d7c0CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
ethanndickson commentedMay 16, 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.
Merge activity
|
05e41b7 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
This PR addresses#121. The underlying bug is still present in macOS, this is just a workaround, so I'm leaving the issue open.
macOS calls
actionForReplacingExtensionwhenever the version string(s) of the system extension living inside the Coder Desktop app bundle change.i.e. When a new version of the app is installed:
activationRequest(it does this on every launch, to see if the NE is installed)actionForReplacingExtensionis called, which can either return.cancelor.replace.didFinishWithResultis called with whether the replacement was successful.(
actionForReplacingExtensionisalways called when developing the app locally, even if the version string(s) don't differ)However, in the linked issue, we note that this replacement process is bug-prone. This bug can be worked around by deleting the system extension (in settings), and reactivating it (such as by relaunching the app).
Therefore, in this PR, when
didFinishWithResultis called following a replacement request, we instead will:deactivationRequest, and wait for it to be successful.activationRequest, and wait for that to be successful.Of note is that wecannot return
.cancelfromactionForReplacingExtensionand then later send adeactivationRequest.deactivationRequestalways searches for a system extension with version string(s) that match the system extension living inside the currently installed app bundle. Therefore, we have to let the replacement take place before attempting to delete it.Also of note is that a successful
deactivationRequestof the system extension deletes the corresponding VPN configuration. This configuration is normally created by logging in, but if the user is already logged in, we'll update the UI to include aReconfigure VPNbutton.I've tested this PR in a fresh macOS 15.4 VM, upgrading from the latest release. I also forced the bug in the linked issue to occur by toggling on the VPN in System Settings before opening the new version of the app for the first time, and going through all the additional prompts did indeed prevent the issue from happening.