-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: add troubleshooting tab and improve extension management #105
Draft
ThomasK33
wants to merge
1
commit into
main
Choose a base branch
from
troubleshooting-tab-and-extension-management
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
# Coder Desktop Development Guide | ||
|
||
## Build & Test Commands | ||
- Build Xcode project: `make` | ||
- Format Swift files: `make fmt` | ||
- Lint Swift files: `make lint` | ||
- Run all tests: `make test` | ||
- Run specific test class: `xcodebuild test -project "Coder Desktop/Coder Desktop.xcodeproj" -scheme "Coder Desktop" -only-testing:"Coder DesktopTests/AgentsTests"` | ||
- Run specific test method: `xcodebuild test -project "Coder Desktop/Coder Desktop.xcodeproj" -scheme "Coder Desktop" -only-testing:"Coder DesktopTests/AgentsTests/agentsWhenVPNOff"` | ||
- Generate Swift from proto: `make proto` | ||
- Watch for project changes: `make watch-gen` | ||
|
||
## Code Style Guidelines | ||
- Use Swift 6.0 for development | ||
- Follow SwiftFormat and SwiftLint rules | ||
- Use Swift's Testing framework for tests (`@Test`, `#expect` directives) | ||
- Group files logically (Views, Models, Extensions) | ||
- Use environment objects for dependency injection | ||
- Prefer async/await over completion handlers | ||
- Use clear, descriptive naming for functions and variables | ||
- Implement proper error handling with Swift's throwing functions | ||
- Tests should use descriptive names reflecting what they're testing |
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,9 +7,15 @@ | |
protocol VPNService: ObservableObject { | ||
var state: VPNServiceState { get } | ||
var menuState: VPNMenuState { get } | ||
var sysExtnState: SystemExtensionState { get } | ||
var neState: NetworkExtensionState { get } | ||
Comment on lines
+10
to
+11
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should expose these, I think it's sufficient to rely on specific EDIT: It's probably better to, actually, but I wish we didn't :/ |
||
func start() async | ||
func stop() async | ||
func configureTunnelProviderProtocol(proto: NETunnelProviderProtocol?) | ||
func uninstall() async -> Bool | ||
func installExtension() async | ||
func disableExtension() async -> Bool | ||
func enableExtension() async -> Bool | ||
} | ||
|
||
enum VPNServiceState: Equatable { | ||
|
@@ -114,6 +120,179 @@ | |
} | ||
} | ||
|
||
func uninstall() async -> Bool { | ||
logger.info("Uninstalling VPN system extension...") | ||
|
||
// First stop any active VPN tunnels | ||
if tunnelState == .connected || tunnelState == .connecting { | ||
await stop() | ||
|
||
// Wait for tunnel state to actually change to disabled | ||
let startTime = Date() | ||
let timeout = TimeInterval(10) // 10 seconds timeout | ||
|
||
while tunnelState != .disabled { | ||
// Check for timeout | ||
if Date().timeIntervalSince(startTime) > timeout { | ||
logger.warning("Timeout waiting for VPN to disconnect before uninstall") | ||
break | ||
} | ||
|
||
// Wait a bit before checking again | ||
try? await Task.sleep(for: .milliseconds(100)) | ||
} | ||
} | ||
|
||
// Remove network extension configuration | ||
do { | ||
try await removeNetworkExtension() | ||
neState = .unconfigured | ||
tunnelState = .disabled | ||
} catch { | ||
logger.error("Failed to remove network extension configuration: \(error.localizedDescription)") | ||
// Continue with deregistration even if removing network extension failed | ||
} | ||
|
||
// Deregister the system extension | ||
let success = await deregisterSystemExtension() | ||
if success { | ||
logger.info("Successfully uninstalled VPN system extension") | ||
sysExtnState = .uninstalled | ||
} else { | ||
logger.error("Failed to uninstall VPN system extension") | ||
sysExtnState = .failed("Deregistration failed") | ||
} | ||
|
||
return success | ||
} | ||
|
||
func installExtension() async { | ||
logger.info("Installing VPN system extension...") | ||
|
||
// Install the system extension | ||
installSystemExtension() | ||
|
||
// We don't need to await here since the installSystemExtension method | ||
// uses a delegate callback system to update the state | ||
} | ||
|
||
func disableExtension() async -> Bool { | ||
logger.info("Disabling VPN network extension without uninstalling...") | ||
|
||
// First stop any active VPN tunnel | ||
if tunnelState == .connected || tunnelState == .connecting { | ||
await stop() | ||
} | ||
|
||
// Remove network extension configuration but keep the system extension | ||
do { | ||
try await removeNetworkExtension() | ||
neState = .unconfigured | ||
tunnelState = .disabled | ||
logger.info("Successfully disabled network extension") | ||
return true | ||
} catch { | ||
logger.error("Failed to disable network extension: \(error.localizedDescription)") | ||
neState = .failed(error.localizedDescription) | ||
return false | ||
} | ||
} | ||
|
||
func enableExtension() async -> Bool { | ||
logger.info("Enabling VPN network extension...") | ||
|
||
// Ensure system extension is installed | ||
let extensionInstalled = await ensureSystemExtensionInstalled() | ||
if !extensionInstalled { | ||
return false | ||
} | ||
|
||
// Get the initial state for comparison | ||
let initialNeState = neState | ||
|
||
// Directly inject AppState dependency to call reconfigure | ||
if let appState = (NSApp.delegate as? AppDelegate)?.state, appState.hasSession { | ||
appState.reconfigure() | ||
} else { | ||
// No valid session, the user likely needs to log in again | ||
await MainActor.run { | ||
NSApp.sendAction(#selector(NSApplication.showLoginWindow), to: nil, from: nil) | ||
} | ||
} | ||
|
||
// Wait for network extension state to change | ||
let stateChanged = await waitForNetworkExtensionChange(from: initialNeState) | ||
if !stateChanged { | ||
return false | ||
} | ||
|
||
logger.info("Network extension was reconfigured successfully") | ||
|
||
// Try to connect to VPN if needed | ||
return await tryConnectAfterReconfiguration() | ||
} | ||
|
||
private func ensureSystemExtensionInstalled() async -> Bool { | ||
if sysExtnState != .installed { | ||
installSystemExtension() | ||
// Wait for the system extension to be installed | ||
for _ in 0 ..< 30 { // Wait up to 3 seconds | ||
if sysExtnState == .installed { | ||
break | ||
} | ||
try? await Task.sleep(for: .milliseconds(100)) | ||
} | ||
|
||
if sysExtnState != .installed { | ||
logger.error("Failed to install system extension during enableExtension") | ||
return false | ||
} | ||
} | ||
return true | ||
} | ||
|
||
private func waitForNetworkExtensionChange(from initialState: NetworkExtensionState) async -> Bool { | ||
// Wait for network extension state to change from the initial state | ||
for _ in 0 ..< 30 { // Wait up to 3 seconds | ||
// If the state changes at all from the initial state, we consider reconfiguration successful | ||
if neState != initialState || neState == .enabled { | ||
return true | ||
} | ||
try? await Task.sleep(for: .milliseconds(100)) | ||
} | ||
|
||
logger.error("Network extension configuration didn't change after reconfiguration request") | ||
return false | ||
} | ||
|
||
private func tryConnectAfterReconfiguration() async -> Bool { | ||
// If already enabled, we're done | ||
if neState == .enabled { | ||
logger.info("Network extension enabled successfully") | ||
return true | ||
} | ||
|
||
// Wait a bit longer for the configuration to be fully applied | ||
try? await Task.sleep(for: .milliseconds(500)) | ||
|
||
// If the extension is in a state we can work with, try to start the VPN | ||
if case .failed = neState { | ||
logger.error("Network extension in failed state, skipping auto-connection") | ||
} else if neState != .unconfigured { | ||
logger.info("Attempting to automatically connect to VPN after reconfiguration") | ||
await start() | ||
|
||
if tunnelState == .connecting || tunnelState == .connected { | ||
logger.info("VPN connection started successfully after reconfiguration") | ||
return true | ||
} | ||
} | ||
|
||
// If we get here, the extension was reconfigured but not successfully enabled | ||
// Since configuration was successful, return true so user can manually connect | ||
return true | ||
} | ||
|
||
func onExtensionPeerUpdate(_ data: Data) { | ||
logger.info("network extension peer update") | ||
do { | ||
|
Oops, something went wrong.
Oops, something went wrong.
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.
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.
Why would we use this over
@Environment(\.openWindow)
?