- Notifications
You must be signed in to change notification settings - Fork3
feat: add troubleshooting tab and improve extension management#105
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
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
960cacf tofed46bdCompare
ethanndickson left a comment• 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.
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.
Add "Stop VPN on Quit" setting to control VPN behavior when application closes
I wrote this and it didn't even change it💀
Whilst I am impressed it even compiled, the quality definitely leaves a lot to be desired..
| // Subscribe to reconfiguration requests | ||
| NotificationCenter.default.addObserver( | ||
| self, | ||
| selector: #selector(networkExtensionNeedsReconfiguration(_:)), | ||
| name: .networkExtensionNeedsReconfiguration, | ||
| object: 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.
What's the point of making this a custom event and sending it ourselves? Why wouldn't we just callreconfigure?
| extension NSApplication { | ||
| @objc func showLoginWindow() { | ||
| NSApp.sendAction(#selector(NSWindowController.showWindow(_:)), to: nil, from: Windows.login.rawValue) | ||
| } | ||
| } |
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)?
| @@ -8,7 +8,7 @@ class AppState: ObservableObject { | |||
| let appId = Bundle.main.bundleIdentifier! | |||
| // Stored in UserDefaults | |||
| @Published private(set) var hasSession: Bool { | |||
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 are we getting rid of this? The whole point is the class maintains the invariant that ifhasSession is true, the auth details exist.
| // A dedicated delegate class for system extension deregistration | ||
| private class DeregistrationDelegate: NSObject, OSSystemExtensionRequestDelegate { |
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 should just use the existing delegate?
| let initialNeState = neState | ||
| // Post a notification that the app should reconfigure | ||
| NotificationCenter.default.post(name: .networkExtensionNeedsReconfiguration, object: 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 either callsstate.reconfigure or opens the login window. Both things we could do from this function already.
| var sysExtnState: SystemExtensionState { get } | ||
| var neState: NetworkExtensionState { get } |
ethanndicksonMar 11, 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.
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.
I don't think we should expose these, I think it's sufficient to rely on specificVPNServiceStates.
EDIT: It's probably better to, actually, but I wish we didn't :/
| if tunnelState == .connected || tunnelState == .connecting { | ||
| await stop() | ||
| } |
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 just starts the stop operation. It's not complete until thetunnelState changes todisabled.
| @@ -1,13 +1,16 @@ | |||
| import LaunchAtLogin | |||
| import SwiftUI | |||
| struct GeneralTab: View { | |||
| struct GeneralTab<VPN: VPNService>: View { | |||
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's this need to be here?
| isProcessing = false | ||
| if !success { | ||
| networkExtensionError = "Failed to enable network extension. Check logs for details." |
ethanndicksonMar 11, 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.
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 isn't a reasonable request on macOS.enableExtension is also not successful if the configuration didn't change, in which case we shouldn't show an error.
- Add troubleshooting tab to settings with system and network extension controls- Address review feedback related to extension management: - Replace custom notifications with direct method calls for reconfiguration - Restore proper encapsulation for session state management - Improve extension uninstallation with proper state monitoring - Fix deregistration to use existing delegate pattern - Enhance error handling for network extension operations🤖 Generated with [Claude Code](https://claude.ai/code)Co-Authored-By: Claude <noreply@anthropic.com>Change-Id: I947bae1bc7680bd1f670245e4541a95619ab41eeSigned-off-by: Thomas Kosiewski <tk@coder.com>
fed46bd to24d5538Compare
Uh oh!
There was an error while loading.Please reload this page.
Summary
Test plan
🤖 Generated withClaude Code