fix: manually upgrade system extension#158
Merged
Merged
Conversation
ethanndickson
force-pushed
the
ethan/manually-upgrade-network-extension
branch
2 times, most recently
from
888fd0e to
428495e
Compare
There was a problem hiding this comment.
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)")
ethanndickson
commented
May 13, 2025
| } | ||
| } | ||
|
|
||
| let extensionBundle: Bundle = { |
Member
Author
There was a problem hiding this comment.
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.
| .padding(.vertical, Theme.Size.trayPadding) | ||
| .frame(maxWidth: .infinity) | ||
| default: | ||
| case (.connected, true): |
Member
Author
There was a problem hiding this comment.
Same behaviour, just more explicit.
ethanndickson
changed the title
ethanndickson
force-pushed
the
ethan/manually-upgrade-network-extension
branch
from
428495e to
828d7c0
Compare
deansheather
approved these changes
May 14, 2025
Member
Author
Merge activity
|
ethanndickson
added a commit
that referenced
this pull request
Jul 24, 2025
I undid the work of #114 in #158, by disabling the VPN toggle when the network extension was unconfigured. When signed out, the network extension is unconfigured. This PR adds a special exception to make the VPN toggle always clickable when signed out, as it has special behaviour when signed out. It also adds a proper regression test. A slightly different regression test existed, but it didn't account for cases where the VPN state is one that would normally disable the toggle.
This file contains hidden or 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
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.
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.(
actionForReplacingExtensionis always 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 we cannot 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.